| Message ID | 20191126122324.v3rj7oscvjobhjxo@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: pic32: Fix an error code in pic32_spi_dma_prep() | expand |
Hi Dan, On Tue, Nov 26, 2019 at 1:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > We accidentally return success on this error path. > > Fixes: eb7e6dc6d9ff ("spi: pic32: Retire dma_request_slave_channel_compat()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/spi/spi-pic32.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c > index 156961b4ca86..93fb95073522 100644 > --- a/drivers/spi/spi-pic32.c > +++ b/drivers/spi/spi-pic32.c > @@ -633,7 +633,8 @@ static int pic32_spi_dma_prep(struct pic32_spi *pic32s, struct device *dev) > goto out_err; > } > > - if (pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE)) > + ret = pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE); > + if (ret) > goto out_err; This used to be non-fatal, as DMA was optional, cfr. the comment: /* optional DMA support */ ret = pic32_spi_dma_prep(pic32s, &pdev->dev); if (ret) goto err_bailout; However, as of the aforementioned commit, DMA is no longer optional, and probe will fail instead of falling back to PIO on DMA init failure... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 26/11/2019 15.27, Geert Uytterhoeven wrote: > Hi Dan, > > On Tue, Nov 26, 2019 at 1:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: >> We accidentally return success on this error path. >> >> Fixes: eb7e6dc6d9ff ("spi: pic32: Retire dma_request_slave_channel_compat()") >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> --- >> drivers/spi/spi-pic32.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c >> index 156961b4ca86..93fb95073522 100644 >> --- a/drivers/spi/spi-pic32.c >> +++ b/drivers/spi/spi-pic32.c >> @@ -633,7 +633,8 @@ static int pic32_spi_dma_prep(struct pic32_spi *pic32s, struct device *dev) >> goto out_err; >> } >> >> - if (pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE)) >> + ret = pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE); >> + if (ret) >> goto out_err; > > This used to be non-fatal, as DMA was optional, cfr. the comment: > > /* optional DMA support */ > ret = pic32_spi_dma_prep(pic32s, &pdev->dev); > if (ret) > goto err_bailout; > > However, as of the aforementioned commit, DMA is no longer optional, > and probe will fail instead of falling back to PIO on DMA init failure... It is still optional. It only handles deferred probing. If the DMA request fails then it returns 0 and the driver falls back to PIO. And this is why I have not changed the pic32_spi_dma_config() error handling. > > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 26/11/2019 14.23, Dan Carpenter wrote: > We accidentally return success on this error path. > > Fixes: eb7e6dc6d9ff ("spi: pic32: Retire dma_request_slave_channel_compat()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/spi/spi-pic32.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c > index 156961b4ca86..93fb95073522 100644 > --- a/drivers/spi/spi-pic32.c > +++ b/drivers/spi/spi-pic32.c > @@ -633,7 +633,8 @@ static int pic32_spi_dma_prep(struct pic32_spi *pic32s, struct device *dev) > goto out_err; > } > > - if (pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE)) > + ret = pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE); > + if (ret) > goto out_err; I have intentionally left it like this to fall back to PIO mode in case of an error as the original implementation did. With commit eb7e6dc6d9ff the driver _only_ handles the -EPROBE_DEFER, in all other cases it falls back to PIO mode. > > /* DMA chnls allocated and prepared */ > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Péter, On Tue, Nov 26, 2019 at 2:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > On 26/11/2019 15.27, Geert Uytterhoeven wrote: > > On Tue, Nov 26, 2019 at 1:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > >> We accidentally return success on this error path. > >> > >> Fixes: eb7e6dc6d9ff ("spi: pic32: Retire dma_request_slave_channel_compat()") > >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > >> --- > >> drivers/spi/spi-pic32.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c > >> index 156961b4ca86..93fb95073522 100644 > >> --- a/drivers/spi/spi-pic32.c > >> +++ b/drivers/spi/spi-pic32.c > >> @@ -633,7 +633,8 @@ static int pic32_spi_dma_prep(struct pic32_spi *pic32s, struct device *dev) > >> goto out_err; > >> } > >> > >> - if (pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE)) > >> + ret = pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE); > >> + if (ret) > >> goto out_err; > > > > This used to be non-fatal, as DMA was optional, cfr. the comment: > > > > /* optional DMA support */ > > ret = pic32_spi_dma_prep(pic32s, &pdev->dev); > > if (ret) > > goto err_bailout; > > > > However, as of the aforementioned commit, DMA is no longer optional, > > and probe will fail instead of falling back to PIO on DMA init failure... > > It is still optional. It only handles deferred probing. If the DMA > request fails then it returns 0 and the driver falls back to PIO. Oops, I missed that ret is only set to a non-zero value in case of probe deferral. Sorry for that, seen too many broken patches today ;-) > And this is why I have not changed the pic32_spi_dma_config() error > handling. Right. So Dan's patch is wrong, as it makes this a hard failure, preventing fallback to PIO. Gr{oetje,eeting}s, Geert
Hi Geet, On 26/11/2019 15.38, Geert Uytterhoeven wrote: > Hi Péter, > > On Tue, Nov 26, 2019 at 2:32 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> On 26/11/2019 15.27, Geert Uytterhoeven wrote: >>> On Tue, Nov 26, 2019 at 1:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: >>>> We accidentally return success on this error path. >>>> >>>> Fixes: eb7e6dc6d9ff ("spi: pic32: Retire dma_request_slave_channel_compat()") >>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>>> --- >>>> drivers/spi/spi-pic32.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c >>>> index 156961b4ca86..93fb95073522 100644 >>>> --- a/drivers/spi/spi-pic32.c >>>> +++ b/drivers/spi/spi-pic32.c >>>> @@ -633,7 +633,8 @@ static int pic32_spi_dma_prep(struct pic32_spi *pic32s, struct device *dev) >>>> goto out_err; >>>> } >>>> >>>> - if (pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE)) >>>> + ret = pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE); >>>> + if (ret) >>>> goto out_err; >>> >>> This used to be non-fatal, as DMA was optional, cfr. the comment: >>> >>> /* optional DMA support */ >>> ret = pic32_spi_dma_prep(pic32s, &pdev->dev); >>> if (ret) >>> goto err_bailout; >>> >>> However, as of the aforementioned commit, DMA is no longer optional, >>> and probe will fail instead of falling back to PIO on DMA init failure... >> >> It is still optional. It only handles deferred probing. If the DMA >> request fails then it returns 0 and the driver falls back to PIO. > > Oops, I missed that ret is only set to a non-zero value in case of > probe deferral. > > Sorry for that, seen too many broken patches today ;-) You made me double check the patch to make sure I'm not contributing to the broken patch count ;) >> And this is why I have not changed the pic32_spi_dma_config() error >> handling. > > Right. So Dan's patch is wrong, as it makes this a hard failure, preventing > fallback to PIO. It is unlikely that it would fail, but probably it worth considering to add a warning in case it happens. > > Gr{oetje,eeting}s, > > Geert > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Ah. Thanks Peter. regards, dan carpenter
diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c index 156961b4ca86..93fb95073522 100644 --- a/drivers/spi/spi-pic32.c +++ b/drivers/spi/spi-pic32.c @@ -633,7 +633,8 @@ static int pic32_spi_dma_prep(struct pic32_spi *pic32s, struct device *dev) goto out_err; } - if (pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE)) + ret = pic32_spi_dma_config(pic32s, DMA_SLAVE_BUSWIDTH_1_BYTE); + if (ret) goto out_err; /* DMA chnls allocated and prepared */
We accidentally return success on this error path. Fixes: eb7e6dc6d9ff ("spi: pic32: Retire dma_request_slave_channel_compat()") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/spi/spi-pic32.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)