* [PATCH] spi: atmel: use managed resource for gpio chip select
@ 2016-09-21 8:20 ` Geert Uytterhoeven
0 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2016-09-21 8:20 UTC (permalink / raw)
To: linux-arm-kernel
Hi Nicolas,
On Wed, Sep 21, 2016 at 9:55 AM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> Use the managed gpio CS pin request so that we avoid having trouble
> in the cleanup code.
> In fact, if module was configured with DT, cleanup code released
> invalid pin. Since resource wasn't freed, module cannot be reinserted.
>
> Reported-by: Alexander Morozov <linux@meltdown.ru>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> drivers/spi/spi-atmel.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 8feac599e9ab..4e3f2345844a 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -1248,7 +1248,8 @@ static int atmel_spi_setup(struct spi_device *spi)
> return -ENOMEM;
>
> if (as->use_cs_gpios) {
> - ret = gpio_request(npcs_pin, dev_name(&spi->dev));
> + ret = devm_gpio_request(&spi->dev,
> + npcs_pin, dev_name(&spi->dev));
Note that spi_master.setup() can be called multiple times during the lifetime
of the spi_device.
> if (ret) {
> kfree(asd);
> return ret;
> @@ -1471,13 +1472,11 @@ static int atmel_spi_transfer_one_message(struct spi_master *master,
> static void atmel_spi_cleanup(struct spi_device *spi)
> {
> struct atmel_spi_device *asd = spi->controller_state;
> - unsigned gpio = (unsigned long) spi->controller_data;
>
> if (!asd)
> return;
>
> spi->controller_state = NULL;
> - gpio_free(gpio);
> kfree(asd);
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: atmel: use managed resource for gpio chip select
@ 2016-09-21 8:20 ` Geert Uytterhoeven
0 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2016-09-21 8:20 UTC (permalink / raw)
To: Nicolas Ferre
Cc: linux, Mark Brown, Alexandre Belloni, Boris BREZILLON, linux-spi,
Ludovic Desroches, linux-arm-kernel, linux-kernel,
Cyrille Pitchen, Wenyou Yang
Hi Nicolas,
On Wed, Sep 21, 2016 at 9:55 AM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> Use the managed gpio CS pin request so that we avoid having trouble
> in the cleanup code.
> In fact, if module was configured with DT, cleanup code released
> invalid pin. Since resource wasn't freed, module cannot be reinserted.
>
> Reported-by: Alexander Morozov <linux@meltdown.ru>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> drivers/spi/spi-atmel.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 8feac599e9ab..4e3f2345844a 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -1248,7 +1248,8 @@ static int atmel_spi_setup(struct spi_device *spi)
> return -ENOMEM;
>
> if (as->use_cs_gpios) {
> - ret = gpio_request(npcs_pin, dev_name(&spi->dev));
> + ret = devm_gpio_request(&spi->dev,
> + npcs_pin, dev_name(&spi->dev));
Note that spi_master.setup() can be called multiple times during the lifetime
of the spi_device.
> if (ret) {
> kfree(asd);
> return ret;
> @@ -1471,13 +1472,11 @@ static int atmel_spi_transfer_one_message(struct spi_master *master,
> static void atmel_spi_cleanup(struct spi_device *spi)
> {
> struct atmel_spi_device *asd = spi->controller_state;
> - unsigned gpio = (unsigned long) spi->controller_data;
>
> if (!asd)
> return;
>
> spi->controller_state = NULL;
> - gpio_free(gpio);
> kfree(asd);
> }
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: atmel: use managed resource for gpio chip select
@ 2016-11-07 13:54 ` Nicolas Ferre
0 siblings, 0 replies; 8+ messages in thread
From: Nicolas Ferre @ 2016-11-07 13:54 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux, Mark Brown, Alexandre Belloni, Boris BREZILLON, linux-spi,
Ludovic Desroches, linux-arm-kernel, linux-kernel,
Cyrille Pitchen, Wenyou Yang
Le 21/09/2016 à 10:20, Geert Uytterhoeven a écrit :
> Hi Nicolas,
>
> On Wed, Sep 21, 2016 at 9:55 AM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>> Use the managed gpio CS pin request so that we avoid having trouble
>> in the cleanup code.
>> In fact, if module was configured with DT, cleanup code released
>> invalid pin. Since resource wasn't freed, module cannot be reinserted.
>>
>> Reported-by: Alexander Morozov <linux@meltdown.ru>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>> drivers/spi/spi-atmel.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
>> index 8feac599e9ab..4e3f2345844a 100644
>> --- a/drivers/spi/spi-atmel.c
>> +++ b/drivers/spi/spi-atmel.c
>> @@ -1248,7 +1248,8 @@ static int atmel_spi_setup(struct spi_device *spi)
>> return -ENOMEM;
>>
>> if (as->use_cs_gpios) {
>> - ret = gpio_request(npcs_pin, dev_name(&spi->dev));
>> + ret = devm_gpio_request(&spi->dev,
>> + npcs_pin, dev_name(&spi->dev));
>
> Note that spi_master.setup() can be called multiple times during the lifetime
> of the spi_device.
Sure, this is what I read in include/linux/spi/spi.h "It's always safe
to call this unless transfers are pending on the device whose settings
are being modified."
It also means that the whole memory allocation for devices that is done
a few lines above this gpio request is also completely wrong... This
function needs a serious refactoring.
Thanks for the heads-up.
Best regards,
>> if (ret) {
>> kfree(asd);
>> return ret;
>> @@ -1471,13 +1472,11 @@ static int atmel_spi_transfer_one_message(struct spi_master *master,
>> static void atmel_spi_cleanup(struct spi_device *spi)
>> {
>> struct atmel_spi_device *asd = spi->controller_state;
>> - unsigned gpio = (unsigned long) spi->controller_data;
>>
>> if (!asd)
>> return;
>>
>> spi->controller_state = NULL;
>> - gpio_free(gpio);
>> kfree(asd);
>> }
>
> 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
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] spi: atmel: use managed resource for gpio chip select
@ 2016-11-07 13:54 ` Nicolas Ferre
0 siblings, 0 replies; 8+ messages in thread
From: Nicolas Ferre @ 2016-11-07 13:54 UTC (permalink / raw)
To: linux-arm-kernel
Le 21/09/2016 ? 10:20, Geert Uytterhoeven a ?crit :
> Hi Nicolas,
>
> On Wed, Sep 21, 2016 at 9:55 AM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>> Use the managed gpio CS pin request so that we avoid having trouble
>> in the cleanup code.
>> In fact, if module was configured with DT, cleanup code released
>> invalid pin. Since resource wasn't freed, module cannot be reinserted.
>>
>> Reported-by: Alexander Morozov <linux@meltdown.ru>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>> drivers/spi/spi-atmel.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
>> index 8feac599e9ab..4e3f2345844a 100644
>> --- a/drivers/spi/spi-atmel.c
>> +++ b/drivers/spi/spi-atmel.c
>> @@ -1248,7 +1248,8 @@ static int atmel_spi_setup(struct spi_device *spi)
>> return -ENOMEM;
>>
>> if (as->use_cs_gpios) {
>> - ret = gpio_request(npcs_pin, dev_name(&spi->dev));
>> + ret = devm_gpio_request(&spi->dev,
>> + npcs_pin, dev_name(&spi->dev));
>
> Note that spi_master.setup() can be called multiple times during the lifetime
> of the spi_device.
Sure, this is what I read in include/linux/spi/spi.h "It's always safe
to call this unless transfers are pending on the device whose settings
are being modified."
It also means that the whole memory allocation for devices that is done
a few lines above this gpio request is also completely wrong... This
function needs a serious refactoring.
Thanks for the heads-up.
Best regards,
>> if (ret) {
>> kfree(asd);
>> return ret;
>> @@ -1471,13 +1472,11 @@ static int atmel_spi_transfer_one_message(struct spi_master *master,
>> static void atmel_spi_cleanup(struct spi_device *spi)
>> {
>> struct atmel_spi_device *asd = spi->controller_state;
>> - unsigned gpio = (unsigned long) spi->controller_data;
>>
>> if (!asd)
>> return;
>>
>> spi->controller_state = NULL;
>> - gpio_free(gpio);
>> kfree(asd);
>> }
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: atmel: use managed resource for gpio chip select
@ 2016-11-07 13:54 ` Nicolas Ferre
0 siblings, 0 replies; 8+ messages in thread
From: Nicolas Ferre @ 2016-11-07 13:54 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-lCH5ugpmZSMox3rIn2DAYQ, Mark Brown, Alexandre Belloni,
Boris BREZILLON, linux-spi, Ludovic Desroches,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Cyrille Pitchen,
Wenyou Yang
Le 21/09/2016 à 10:20, Geert Uytterhoeven a écrit :
> Hi Nicolas,
>
> On Wed, Sep 21, 2016 at 9:55 AM, Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>> Use the managed gpio CS pin request so that we avoid having trouble
>> in the cleanup code.
>> In fact, if module was configured with DT, cleanup code released
>> invalid pin. Since resource wasn't freed, module cannot be reinserted.
>>
>> Reported-by: Alexander Morozov <linux-lCH5ugpmZSMox3rIn2DAYQ@public.gmane.org>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>> drivers/spi/spi-atmel.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
>> index 8feac599e9ab..4e3f2345844a 100644
>> --- a/drivers/spi/spi-atmel.c
>> +++ b/drivers/spi/spi-atmel.c
>> @@ -1248,7 +1248,8 @@ static int atmel_spi_setup(struct spi_device *spi)
>> return -ENOMEM;
>>
>> if (as->use_cs_gpios) {
>> - ret = gpio_request(npcs_pin, dev_name(&spi->dev));
>> + ret = devm_gpio_request(&spi->dev,
>> + npcs_pin, dev_name(&spi->dev));
>
> Note that spi_master.setup() can be called multiple times during the lifetime
> of the spi_device.
Sure, this is what I read in include/linux/spi/spi.h "It's always safe
to call this unless transfers are pending on the device whose settings
are being modified."
It also means that the whole memory allocation for devices that is done
a few lines above this gpio request is also completely wrong... This
function needs a serious refactoring.
Thanks for the heads-up.
Best regards,
>> if (ret) {
>> kfree(asd);
>> return ret;
>> @@ -1471,13 +1472,11 @@ static int atmel_spi_transfer_one_message(struct spi_master *master,
>> static void atmel_spi_cleanup(struct spi_device *spi)
>> {
>> struct atmel_spi_device *asd = spi->controller_state;
>> - unsigned gpio = (unsigned long) spi->controller_data;
>>
>> if (!asd)
>> return;
>>
>> spi->controller_state = NULL;
>> - gpio_free(gpio);
>> kfree(asd);
>> }
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread