| Message ID | 20240104114619.280513-1-o451686892@gmail.com |
|---|---|
| State | Accepted, archived |
| Commit | 85f3d3de8d3198f216dd3c146a6db6eae3cfff03 |
| Delegated to: | Dario Binacchi |
| Headers | show |
| Series | cmd: sf: Fix sf probe crash | expand |
Hi On Thu, Jan 4, 2024 at 12:46 PM Weizhao Ouyang <o451686892@gmail.com> wrote: > > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > crashes. > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > --- > cmd/sf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/cmd/sf.c b/cmd/sf.c > index 730996c02b..e3866899f6 100644 > --- a/cmd/sf.c > +++ b/cmd/sf.c > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > } > flash = NULL; > if (use_dt) { > - spi_flash_probe_bus_cs(bus, cs, &new); > - flash = dev_get_uclass_priv(new); > + ret = spi_flash_probe_bus_cs(bus, cs, &new); > + if (!ret) > + flash = dev_get_uclass_priv(new); > } else { > flash = spi_flash_probe(bus, cs, speed, mode); > } > -- > 2.39.2 > Reviewed-by: Michael Trimarchi <michael@amarulasolutions.com> Suggest to add documentation of spi_flash_probe_bus_cs in a separated patch Michael
On 1/4/24 12:46, Weizhao Ouyang wrote: > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > crashes. > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > --- > cmd/sf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/cmd/sf.c b/cmd/sf.c > index 730996c02b..e3866899f6 100644 > --- a/cmd/sf.c > +++ b/cmd/sf.c > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > } > flash = NULL; > if (use_dt) { > - spi_flash_probe_bus_cs(bus, cs, &new); > - flash = dev_get_uclass_priv(new); > + ret = spi_flash_probe_bus_cs(bus, cs, &new); if (ret) return ret; don't you want to rather propagate that error? Thanks, Michal
On Thu, Jan 4, 2024 at 8:00 PM Michal Simek <michal.simek@amd.com> wrote: > > > > On 1/4/24 12:46, Weizhao Ouyang wrote: > > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > > crashes. > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > --- > > cmd/sf.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/cmd/sf.c b/cmd/sf.c > > index 730996c02b..e3866899f6 100644 > > --- a/cmd/sf.c > > +++ b/cmd/sf.c > > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > > } > > flash = NULL; > > if (use_dt) { > > - spi_flash_probe_bus_cs(bus, cs, &new); > > - flash = dev_get_uclass_priv(new); > > + ret = spi_flash_probe_bus_cs(bus, cs, &new); > > if (ret) > return ret; > > don't you want to rather propagate that error? > Well, since the spi_flash is empty, the following code will print the error message and return. BR, Weizhao
On 1/4/24 13:15, Weizhao Ouyang wrote: > On Thu, Jan 4, 2024 at 8:00 PM Michal Simek <michal.simek@amd.com> wrote: >> >> >> >> On 1/4/24 12:46, Weizhao Ouyang wrote: >>> Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe >>> crashes. >>> >>> Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> >>> --- >>> cmd/sf.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/cmd/sf.c b/cmd/sf.c >>> index 730996c02b..e3866899f6 100644 >>> --- a/cmd/sf.c >>> +++ b/cmd/sf.c >>> @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) >>> } >>> flash = NULL; >>> if (use_dt) { >>> - spi_flash_probe_bus_cs(bus, cs, &new); >>> - flash = dev_get_uclass_priv(new); >>> + ret = spi_flash_probe_bus_cs(bus, cs, &new); >> >> if (ret) >> return ret; >> >> don't you want to rather propagate that error? >> > > Well, since the spi_flash is empty, the following code will > print the error message and return. And you return 0 which means everything is fine. But is everything fine in this case? Or do you want to see the error? This is command it means you can simply use && and if previous command succeed you can call something else. Thanks, Michal
On Thu, Jan 4, 2024 at 8:21 PM Michal Simek <michal.simek@amd.com> wrote: > > > > On 1/4/24 13:15, Weizhao Ouyang wrote: > > On Thu, Jan 4, 2024 at 8:00 PM Michal Simek <michal.simek@amd.com> wrote: > >> > >> > >> > >> On 1/4/24 12:46, Weizhao Ouyang wrote: > >>> Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > >>> crashes. > >>> > >>> Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > >>> --- > >>> cmd/sf.c | 5 +++-- > >>> 1 file changed, 3 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/cmd/sf.c b/cmd/sf.c > >>> index 730996c02b..e3866899f6 100644 > >>> --- a/cmd/sf.c > >>> +++ b/cmd/sf.c > >>> @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > >>> } > >>> flash = NULL; > >>> if (use_dt) { > >>> - spi_flash_probe_bus_cs(bus, cs, &new); > >>> - flash = dev_get_uclass_priv(new); > >>> + ret = spi_flash_probe_bus_cs(bus, cs, &new); > >> > >> if (ret) > >> return ret; > >> > >> don't you want to rather propagate that error? > >> > > > > Well, since the spi_flash is empty, the following code will > > print the error message and return. > > And you return 0 which means everything is fine. But is everything fine in this > case? Or do you want to see the error? > > This is command it means you can simply use && and if previous command succeed > you can call something else. > Hi Michal, Please check the code that follows this commit snippet: if (!flash) { printf("Failed to initialize SPI flash at %u:%u (error %d)\n", bus, cs, ret); return 1; } it will print the error and return 1 as the return value. BR, Weizhao
Hi On Thu, Jan 4, 2024 at 1:16 PM Weizhao Ouyang <o451686892@gmail.com> wrote: > > On Thu, Jan 4, 2024 at 8:00 PM Michal Simek <michal.simek@amd.com> wrote: > > > > > > > > On 1/4/24 12:46, Weizhao Ouyang wrote: > > > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > > > crashes. > > > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > > --- > > > cmd/sf.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/cmd/sf.c b/cmd/sf.c > > > index 730996c02b..e3866899f6 100644 > > > --- a/cmd/sf.c > > > +++ b/cmd/sf.c > > > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > > > } > > > flash = NULL; > > > if (use_dt) { > > > - spi_flash_probe_bus_cs(bus, cs, &new); > > > - flash = dev_get_uclass_priv(new); > > > + ret = spi_flash_probe_bus_cs(bus, cs, &new); > > > > if (ret) > > return ret; > > > > don't you want to rather propagate that error? > > > > Well, since the spi_flash is empty, the following code will > print the error message and return. > flash = NULL; if (!flash) { printf("Failed to initialize SPI flash at %u:%u (error %d)\n", bus, cs, ret); return 1; } This code does not change error handling that is already present here. Michael > BR, > Weizhao
On Thu, Jan 4, 2024 at 8:29 PM Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote: > > Hi > > On Thu, Jan 4, 2024 at 1:16 PM Weizhao Ouyang <o451686892@gmail.com> wrote: > > > > On Thu, Jan 4, 2024 at 8:00 PM Michal Simek <michal.simek@amd.com> wrote: > > > > > > > > > > > > On 1/4/24 12:46, Weizhao Ouyang wrote: > > > > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > > > > crashes. > > > > > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > > > --- > > > > cmd/sf.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/cmd/sf.c b/cmd/sf.c > > > > index 730996c02b..e3866899f6 100644 > > > > --- a/cmd/sf.c > > > > +++ b/cmd/sf.c > > > > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > > > > } > > > > flash = NULL; > > > > if (use_dt) { > > > > - spi_flash_probe_bus_cs(bus, cs, &new); > > > > - flash = dev_get_uclass_priv(new); > > > > + ret = spi_flash_probe_bus_cs(bus, cs, &new); > > > > > > if (ret) > > > return ret; > > > > > > don't you want to rather propagate that error? > > > > > > > Well, since the spi_flash is empty, the following code will > > print the error message and return. > > > > flash = NULL; > > if (!flash) { > printf("Failed to initialize SPI flash at %u:%u (error %d)\n", > bus, cs, ret); > return 1; > } > > This code does not change error handling that is already present here. Hi Michael, Sorry I didn't get your point. This patch is designed to avoid spi_flash_probe_bus_cs() crash by null pointer, and let the current existing error handling to prompt it. So I'm not trying to change the current error handling. BR, Weizhao > Michael > > > BR, > > Weizhao > > > > -- > Michael Nazzareno Trimarchi > Co-Founder & Chief Executive Officer > M. +39 347 913 2170 > michael@amarulasolutions.com > __________________________________ > > Amarula Solutions BV > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > T. +31 (0)85 111 9172 > info@amarulasolutions.com > www.amarulasolutions.com
Hi On Thu, Jan 4, 2024 at 1:40 PM Weizhao Ouyang <o451686892@gmail.com> wrote: > > On Thu, Jan 4, 2024 at 8:29 PM Michael Nazzareno Trimarchi > <michael@amarulasolutions.com> wrote: > > > > Hi > > > > On Thu, Jan 4, 2024 at 1:16 PM Weizhao Ouyang <o451686892@gmail.com> wrote: > > > > > > On Thu, Jan 4, 2024 at 8:00 PM Michal Simek <michal.simek@amd.com> wrote: > > > > > > > > > > > > > > > > On 1/4/24 12:46, Weizhao Ouyang wrote: > > > > > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > > > > > crashes. > > > > > > > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > > > > --- > > > > > cmd/sf.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/cmd/sf.c b/cmd/sf.c > > > > > index 730996c02b..e3866899f6 100644 > > > > > --- a/cmd/sf.c > > > > > +++ b/cmd/sf.c > > > > > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > > > > > } > > > > > flash = NULL; > > > > > if (use_dt) { > > > > > - spi_flash_probe_bus_cs(bus, cs, &new); > > > > > - flash = dev_get_uclass_priv(new); > > > > > + ret = spi_flash_probe_bus_cs(bus, cs, &new); > > > > > > > > if (ret) > > > > return ret; > > > > > > > > don't you want to rather propagate that error? > > > > > > > > > > Well, since the spi_flash is empty, the following code will > > > print the error message and return. > > > > > > > flash = NULL; > > > > if (!flash) { > > printf("Failed to initialize SPI flash at %u:%u (error %d)\n", > > bus, cs, ret); > > return 1; > > } > > > > This code does not change error handling that is already present here. > > Hi Michael, > > Sorry I didn't get your point. This patch is designed to avoid > spi_flash_probe_bus_cs() crash by null pointer, and let the > current existing error handling to prompt it. So I'm not > trying to change the current error handling. > I have said exactly the same so I agree with you. Michael > BR, > Weizhao > > > Michael > > > > > BR, > > > Weizhao > > > > > > > > -- > > Michael Nazzareno Trimarchi > > Co-Founder & Chief Executive Officer > > M. +39 347 913 2170 > > michael@amarulasolutions.com > > __________________________________ > > > > Amarula Solutions BV > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > > T. +31 (0)85 111 9172 > > info@amarulasolutions.com > > www.amarulasolutions.com
On 1/4/24 13:28, Weizhao Ouyang wrote: > On Thu, Jan 4, 2024 at 8:21 PM Michal Simek <michal.simek@amd.com> wrote: >> >> >> >> On 1/4/24 13:15, Weizhao Ouyang wrote: >>> On Thu, Jan 4, 2024 at 8:00 PM Michal Simek <michal.simek@amd.com> wrote: >>>> >>>> >>>> >>>> On 1/4/24 12:46, Weizhao Ouyang wrote: >>>>> Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe >>>>> crashes. >>>>> >>>>> Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> >>>>> --- >>>>> cmd/sf.c | 5 +++-- >>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/cmd/sf.c b/cmd/sf.c >>>>> index 730996c02b..e3866899f6 100644 >>>>> --- a/cmd/sf.c >>>>> +++ b/cmd/sf.c >>>>> @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) >>>>> } >>>>> flash = NULL; >>>>> if (use_dt) { >>>>> - spi_flash_probe_bus_cs(bus, cs, &new); >>>>> - flash = dev_get_uclass_priv(new); >>>>> + ret = spi_flash_probe_bus_cs(bus, cs, &new); >>>> >>>> if (ret) >>>> return ret; >>>> >>>> don't you want to rather propagate that error? >>>> >>> >>> Well, since the spi_flash is empty, the following code will >>> print the error message and return. >> >> And you return 0 which means everything is fine. But is everything fine in this >> case? Or do you want to see the error? >> >> This is command it means you can simply use && and if previous command succeed >> you can call something else. >> > > Hi Michal, > > Please check the code that follows this commit snippet: > > if (!flash) { > printf("Failed to initialize SPI flash at %u:%u (error %d)\n", > bus, cs, ret); > return 1; > } > > it will print the error and return 1 as the return value. ok. Then good. Acked-by: Michal Simek <michal.simek@amd.com> Thanks, Michal
Gentle ping. Not merged yet. BR, Weizhao On Thu, Jan 4, 2024 at 7:46 PM Weizhao Ouyang <o451686892@gmail.com> wrote: > > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > crashes. > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > --- > cmd/sf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/cmd/sf.c b/cmd/sf.c > index 730996c02b..e3866899f6 100644 > --- a/cmd/sf.c > +++ b/cmd/sf.c > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > } > flash = NULL; > if (use_dt) { > - spi_flash_probe_bus_cs(bus, cs, &new); > - flash = dev_get_uclass_priv(new); > + ret = spi_flash_probe_bus_cs(bus, cs, &new); > + if (!ret) > + flash = dev_get_uclass_priv(new); > } else { > flash = spi_flash_probe(bus, cs, speed, mode); > } > -- > 2.39.2 >
On 04.03.24 15:47, Weizhao Ouyang wrote: > Gentle ping. Not merged yet. > > BR, > Weizhao Hello Dario, that patch is in your patchwork review queue. Could you, please, have a look. Best regards Heinrich > > On Thu, Jan 4, 2024 at 7:46 PM Weizhao Ouyang <o451686892@gmail.com> wrote: >> >> Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe >> crashes. >> >> Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> >> --- >> cmd/sf.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/cmd/sf.c b/cmd/sf.c >> index 730996c02b..e3866899f6 100644 >> --- a/cmd/sf.c >> +++ b/cmd/sf.c >> @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) >> } >> flash = NULL; >> if (use_dt) { >> - spi_flash_probe_bus_cs(bus, cs, &new); >> - flash = dev_get_uclass_priv(new); >> + ret = spi_flash_probe_bus_cs(bus, cs, &new); >> + if (!ret) >> + flash = dev_get_uclass_priv(new); >> } else { >> flash = spi_flash_probe(bus, cs, speed, mode); >> } >> -- >> 2.39.2 >>
Hi On Mon, Mar 4, 2024 at 4:44 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 04.03.24 15:47, Weizhao Ouyang wrote: > > Gentle ping. Not merged yet. > > > > BR, > > Weizhao > > Hello Dario, > > that patch is in your patchwork review queue. Could you, please, have a > look. He is with me, I will ping about all the patches that we need to queue Michael > > Best regards > > Heinrich > > > > > On Thu, Jan 4, 2024 at 7:46 PM Weizhao Ouyang <o451686892@gmail.com> wrote: > >> > >> Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > >> crashes. > >> > >> Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > >> --- > >> cmd/sf.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/cmd/sf.c b/cmd/sf.c > >> index 730996c02b..e3866899f6 100644 > >> --- a/cmd/sf.c > >> +++ b/cmd/sf.c > >> @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > >> } > >> flash = NULL; > >> if (use_dt) { > >> - spi_flash_probe_bus_cs(bus, cs, &new); > >> - flash = dev_get_uclass_priv(new); > >> + ret = spi_flash_probe_bus_cs(bus, cs, &new); > >> + if (!ret) > >> + flash = dev_get_uclass_priv(new); > >> } else { > >> flash = spi_flash_probe(bus, cs, speed, mode); > >> } > >> -- > >> 2.39.2 > >> >
Hi, On 2024-01-04 12:46, Weizhao Ouyang wrote: > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > crashes. > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> This fixes a null pointer dereference when running "sf probe" and there are no spi devices enabled in the device tree for my boards, so: Fixes: 3feea0ba196a ("spi: spi_flash_probe_bus_cs() rely on DT for spi speed and mode") Reviewed-by: Jonas Karlman <jonas@kwiboo.se> Regards, Jonas > --- > cmd/sf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/cmd/sf.c b/cmd/sf.c > index 730996c02b..e3866899f6 100644 > --- a/cmd/sf.c > +++ b/cmd/sf.c > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > } > flash = NULL; > if (use_dt) { > - spi_flash_probe_bus_cs(bus, cs, &new); > - flash = dev_get_uclass_priv(new); > + ret = spi_flash_probe_bus_cs(bus, cs, &new); > + if (!ret) > + flash = dev_get_uclass_priv(new); > } else { > flash = spi_flash_probe(bus, cs, speed, mode); > }
Hi Weizhao, On Fri, Mar 15, 2024 at 7:07 PM Jonas Karlman <jonas@kwiboo.se> wrote: > > Hi, > > On 2024-01-04 12:46, Weizhao Ouyang wrote: > > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe > > crashes. > > > > Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> > > This fixes a null pointer dereference when running "sf probe" and there > are no spi devices enabled in the device tree for my boards, so: > > Fixes: 3feea0ba196a ("spi: spi_flash_probe_bus_cs() rely on DT for spi speed and mode") > > Reviewed-by: Jonas Karlman <jonas@kwiboo.se> > > Regards, > Jonas > > > --- > > cmd/sf.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/cmd/sf.c b/cmd/sf.c > > index 730996c02b..e3866899f6 100644 > > --- a/cmd/sf.c > > +++ b/cmd/sf.c > > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) > > } > > flash = NULL; > > if (use_dt) { > > - spi_flash_probe_bus_cs(bus, cs, &new); > > - flash = dev_get_uclass_priv(new); > > + ret = spi_flash_probe_bus_cs(bus, cs, &new); > > + if (!ret) > > + flash = dev_get_uclass_priv(new); > > } else { > > flash = spi_flash_probe(bus, cs, speed, mode); > > } > Applied to nand-next Thanks and regards Dario
diff --git a/cmd/sf.c b/cmd/sf.c index 730996c02b..e3866899f6 100644 --- a/cmd/sf.c +++ b/cmd/sf.c @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[]) } flash = NULL; if (use_dt) { - spi_flash_probe_bus_cs(bus, cs, &new); - flash = dev_get_uclass_priv(new); + ret = spi_flash_probe_bus_cs(bus, cs, &new); + if (!ret) + flash = dev_get_uclass_priv(new); } else { flash = spi_flash_probe(bus, cs, speed, mode); }
Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe crashes. Signed-off-by: Weizhao Ouyang <o451686892@gmail.com> --- cmd/sf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)