[v4,3/4] serial: mctrl_gpio: enable API usage only for initialized mctrl_gpios struct

Commit Message

Yegor Yefremov May 23, 2016, 7:30 a.m. UTC

From: Yegor Yefremov <[e-mail]>

This workaround is needed for the cases, where mctrl_gpio API is used
before mctrl_gpio_init() was invoked. This happens in 8250 during
console initialization, as the driver sets DTR signal.

Return NULL in case of initialization errors or absence of GPIOLIB.
This way the above workaround can be safely used.

Signed-off-by: Yegor Yefremov <[e-mail]>
---
 drivers/tty/serial/serial_mctrl_gpio.c | 24 +++++++++++++++++++++---
 drivers/tty/serial/serial_mctrl_gpio.h |  2 +-
 2 files changed, 22 insertions(+), 4 deletions(-)

Comments

Uwe Kleine-König May 23, 2016, 9:09 a.m. UTC | #1

Hello Yegor,

On Mon, May 23, 2016 at 09:30:57AM +0200, [e-mail] wrote:
> From: Yegor Yefremov <[e-mail]>
> 
> This workaround is needed for the cases, where mctrl_gpio API is used
> before mctrl_gpio_init() was invoked. This happens in 8250 during
> console initialization, as the driver sets DTR signal.
> 
> Return NULL in case of initialization errors or absence of GPIOLIB.
> This way the above workaround can be safely used.

My sentiments for this patch didn't change: I don't like it. Better make
sure that you already called mctrl_gpio_init before you call any other
mctrl_gpio_* function. Otherwise you prepare bad surprises.

> Signed-off-by: Yegor Yefremov <[e-mail]>
> ---
>  drivers/tty/serial/serial_mctrl_gpio.c | 24 +++++++++++++++++++++---
>  drivers/tty/serial/serial_mctrl_gpio.h |  2 +-
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index a868595..35588c5 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -52,6 +52,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
>  	int value_array[UART_GPIO_MAX];
>  	unsigned int count = 0;
>  
> +	if (gpios == NULL)
> +		return;
> +
>  	for (i = 0; i < UART_GPIO_MAX; i++)
>  		if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
>  			desc_array[count] = gpios->gpio[i];
> @@ -73,6 +76,9 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
>  {
>  	enum mctrl_gpio_idx i;
>  
> +	if (gpios == NULL)
> +		return *mctrl;
> +
>  	for (i = 0; i < UART_GPIO_MAX; i++) {
>  		if (gpios->gpio[i] && !mctrl_gpios_desc[i].dir_out) {
>  			if (gpiod_get_value(gpios->gpio[i]))
> @@ -91,6 +97,9 @@ mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl)
>  {
>  	enum mctrl_gpio_idx i;
>  
> +	if (gpios == NULL)
> +		return *mctrl;
> +
>  	for (i = 0; i < UART_GPIO_MAX; i++) {
>  		if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
>  			if (gpiod_get_value(gpios->gpio[i]))

I could live with the changes above, ...

> @@ -178,7 +187,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>  
>  	gpios = mctrl_gpio_init_noauto(port->dev, idx);
>  	if (IS_ERR(gpios))
> -		return gpios;
> +		return NULL;

..., but this one is definitely bogus. If the UART is probed before the
GPIO driver, you want to pass -EPROBE_DEFER. If the GPIO is already
requested, you also want to forward the error.

>  	gpios->port = port;
>  
> @@ -193,7 +202,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>  			dev_err(port->dev,
>  				"failed to find corresponding irq for %s (idx=%d, err=%d)\n",
>  				mctrl_gpios_desc[i].name, idx, ret);
> -			return ERR_PTR(ret);
> +			return NULL;

Ditto, if a GPIO is used as handshake line and it doesn't have a
corresponding irq, polling must be set up instead of simply ignoring the
missing irq.

>  		}
>  		gpios->irq[i] = ret;
>  
> @@ -209,7 +218,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>  			dev_err(port->dev,
>  				"failed to request irq for %s (idx=%d, err=%d)\n",
>  				mctrl_gpios_desc[i].name, idx, ret);
> -			return ERR_PTR(ret);
> +			return NULL;

ditto.

> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
> index fa000bc..82c5fbf 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.h
> +++ b/drivers/tty/serial/serial_mctrl_gpio.h
> @@ -130,7 +130,7 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
>  static inline
>  struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>  {
> -	return ERR_PTR(-ENOSYS);
> +	return NULL;

With the same reasoning that gpiod_get_optional returns -ENOSYS without
GPIOLIB, this also should return -ENOSYS.

Best regards
Uwe

Yegor Yefremov May 23, 2016, 9:40 a.m. UTC | #2

On Mon, May 23, 2016 at 11:09 AM, Uwe Kleine-König
<[e-mail]> wrote:
> Hello Yegor,
>
> On Mon, May 23, 2016 at 09:30:57AM +0200, [e-mail] wrote:
>> From: Yegor Yefremov <[e-mail]>
>>
>> This workaround is needed for the cases, where mctrl_gpio API is used
>> before mctrl_gpio_init() was invoked. This happens in 8250 during
>> console initialization, as the driver sets DTR signal.
>>
>> Return NULL in case of initialization errors or absence of GPIOLIB.
>> This way the above workaround can be safely used.
>
> My sentiments for this patch didn't change: I don't like it. Better make
> sure that you already called mctrl_gpio_init before you call any other
> mctrl_gpio_* function. Otherwise you prepare bad surprises.

Let's keep everything as before and only check for (gpios == NULL),
that is then indicator, that MCTRL_GPIO API wasn't initialized.

>> Signed-off-by: Yegor Yefremov <[e-mail]>
>> ---
>>  drivers/tty/serial/serial_mctrl_gpio.c | 24 +++++++++++++++++++++---
>>  drivers/tty/serial/serial_mctrl_gpio.h |  2 +-
>>  2 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
>> index a868595..35588c5 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>> @@ -52,6 +52,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
>>       int value_array[UART_GPIO_MAX];
>>       unsigned int count = 0;
>>
>> +     if (gpios == NULL)
>> +             return;
>> +
>>       for (i = 0; i < UART_GPIO_MAX; i++)
>>               if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
>>                       desc_array[count] = gpios->gpio[i];
>> @@ -73,6 +76,9 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
>>  {
>>       enum mctrl_gpio_idx i;
>>
>> +     if (gpios == NULL)
>> +             return *mctrl;
>> +
>>       for (i = 0; i < UART_GPIO_MAX; i++) {
>>               if (gpios->gpio[i] && !mctrl_gpios_desc[i].dir_out) {
>>                       if (gpiod_get_value(gpios->gpio[i]))
>> @@ -91,6 +97,9 @@ mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl)
>>  {
>>       enum mctrl_gpio_idx i;
>>
>> +     if (gpios == NULL)
>> +             return *mctrl;
>> +
>>       for (i = 0; i < UART_GPIO_MAX; i++) {
>>               if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
>>                       if (gpiod_get_value(gpios->gpio[i]))
>
> I could live with the changes above, ...

OK

>> @@ -178,7 +187,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>>
>>       gpios = mctrl_gpio_init_noauto(port->dev, idx);
>>       if (IS_ERR(gpios))
>> -             return gpios;
>> +             return NULL;
>
> ..., but this one is definitely bogus. If the UART is probed before the
> GPIO driver, you want to pass -EPROBE_DEFER. If the GPIO is already
> requested, you also want to forward the error.
>
>>       gpios->port = port;
>>
>> @@ -193,7 +202,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>>                       dev_err(port->dev,
>>                               "failed to find corresponding irq for %s (idx=%d, err=%d)\n",
>>                               mctrl_gpios_desc[i].name, idx, ret);
>> -                     return ERR_PTR(ret);
>> +                     return NULL;
>
> Ditto, if a GPIO is used as handshake line and it doesn't have a
> corresponding irq, polling must be set up instead of simply ignoring the
> missing irq.
>
>>               }
>>               gpios->irq[i] = ret;
>>
>> @@ -209,7 +218,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>>                       dev_err(port->dev,
>>                               "failed to request irq for %s (idx=%d, err=%d)\n",
>>                               mctrl_gpios_desc[i].name, idx, ret);
>> -                     return ERR_PTR(ret);
>> +                     return NULL;
>
> ditto.
>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
>> index fa000bc..82c5fbf 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.h
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.h
>> @@ -130,7 +130,7 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
>>  static inline
>>  struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>>  {
>> -     return ERR_PTR(-ENOSYS);
>> +     return NULL;
>
> With the same reasoning that gpiod_get_optional returns -ENOSYS without
> GPIOLIB, this also should return -ENOSYS.

OK. What do think about such workaround in
drivers/tty/serial/8250/8250_core.c for not breaking the UART
initialization, when GPIOLIB is disabled?

if (IS_ERR(uart->gpios) && IS_ENABLED(CONFIG_GPIOLIB))
        return real error;

Yegor
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König May 23, 2016, 9:49 a.m. UTC | #3

Hello Yegor,

On Mon, May 23, 2016 at 11:40:44AM +0200, Yegor Yefremov wrote:
> > With the same reasoning that gpiod_get_optional returns -ENOSYS without
> > GPIOLIB, this also should return -ENOSYS.
> 
> OK. What do think about such workaround in
> drivers/tty/serial/8250/8250_core.c for not breaking the UART
> initialization, when GPIOLIB is disabled?
> 
> if (IS_ERR(uart->gpios) && IS_ENABLED(CONFIG_GPIOLIB))
>         return real error;

If your 8250 has a rts gpio specified in the device tree, what do you
expect from a kernel without GPIOLIB support?

If it's important that RTS is setup correctly, you should return
failure above. If it's not important that RTS is handled correctly, then
think again what this implies for a GPIOLIB=y .config. Can you just drop
RTS handling then?

Best regards
Uwe

Peter Hurley May 23, 2016, 5:51 p.m. UTC | #4

On 05/23/2016 02:49 AM, Uwe Kleine-König wrote:
> Hello Yegor,
> 
> On Mon, May 23, 2016 at 11:40:44AM +0200, Yegor Yefremov wrote:
>>> With the same reasoning that gpiod_get_optional returns -ENOSYS without
>>> GPIOLIB, this also should return -ENOSYS.
>>
>> OK. What do think about such workaround in
>> drivers/tty/serial/8250/8250_core.c for not breaking the UART
>> initialization, when GPIOLIB is disabled?
>>
>> if (IS_ERR(uart->gpios) && IS_ENABLED(CONFIG_GPIOLIB))
>>         return real error;

Don't assign uart->gpios directly.

	gpios = mctrl_gpio_init(&uart->port, 0);
	if (IS_ERR(gpios)) {
		if (PTR_ERR(gpios) != -ENOSYS)
			return PTR_ERR(gpios);
	} else
		mctrl->gpios = gpios;


> If your 8250 has a rts gpio specified in the device tree, what do you
> expect from a kernel without GPIOLIB support?
> 
> If it's important that RTS is setup correctly, you should return
> failure above. If it's not important that RTS is handled correctly, then
> think again what this implies for a GPIOLIB=y .config. Can you just drop
> RTS handling then?

I want to emit a diagnostic for:
1. an actual resource acquisition error
2. a misconfiguration

The driver should fail probe for actual resource acquisition error.
The driver should continue anyway for misconfiguration with mctrl
gpio api calls nop'd.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Hurley May 23, 2016, 6:02 p.m. UTC | #5

On 05/23/2016 02:09 AM, Uwe Kleine-König wrote:
>>  	gpios->port = port;
>> >  
>> > @@ -193,7 +202,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>> >  			dev_err(port->dev,
>> >  				"failed to find corresponding irq for %s (idx=%d, err=%d)\n",
>> >  				mctrl_gpios_desc[i].name, idx, ret);
>> > -			return ERR_PTR(ret);
>> > +			return NULL;
> 
> Ditto, if a GPIO is used as handshake line and it doesn't have a
> corresponding irq, polling must be set up instead of simply ignoring the
> missing irq.

I agree that none of the "return NULL" changes are necessary.

However, note that it's not really possible to recover here by polling;
some of the gpios may have been claimed and some not when the irq
claim failure occurs.

How to know which gpios were claimed?

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König May 24, 2016, 8:09 a.m. UTC | #6

Hello Peter,

On Mon, May 23, 2016 at 11:02:53AM -0700, Peter Hurley wrote:
> On 05/23/2016 02:09 AM, Uwe Kleine-König wrote:
> >>  	gpios->port = port;
> >> >  
> >> > @@ -193,7 +202,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
> >> >  			dev_err(port->dev,
> >> >  				"failed to find corresponding irq for %s (idx=%d, err=%d)\n",
> >> >  				mctrl_gpios_desc[i].name, idx, ret);
> >> > -			return ERR_PTR(ret);
> >> > +			return NULL;
> > 
> > Ditto, if a GPIO is used as handshake line and it doesn't have a
> > corresponding irq, polling must be set up instead of simply ignoring the
> > missing irq.
> 
> I agree that none of the "return NULL" changes are necessary.
> 
> However, note that it's not really possible to recover here by polling;
> some of the gpios may have been claimed and some not when the irq
> claim failure occurs.
> How to know which gpios were claimed?

I don't understand this. You cannot recover if a GPIO is used by a
different driver/device. If a given GPIO doesn't support triggering an
irq, polling can help, it must of course be implemented correctly.
And if gpio_mctrl implements this (which is the right thing to do, as it
is independant from the actual driver) all necessary knowledge is there.

Best regards
Uwe

Peter Hurley May 24, 2016, 2:40 p.m. UTC | #7

On 05/24/2016 01:09 AM, Uwe Kleine-König wrote:
> Hello Peter,
> 
> On Mon, May 23, 2016 at 11:02:53AM -0700, Peter Hurley wrote:
>> On 05/23/2016 02:09 AM, Uwe Kleine-König wrote:
>>>>  	gpios->port = port;
>>>>>  
>>>>> @@ -193,7 +202,7 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
>>>>>  			dev_err(port->dev,
>>>>>  				"failed to find corresponding irq for %s (idx=%d, err=%d)\n",
>>>>>  				mctrl_gpios_desc[i].name, idx, ret);
>>>>> -			return ERR_PTR(ret);
>>>>> +			return NULL;
>>>
>>> Ditto, if a GPIO is used as handshake line and it doesn't have a
>>> corresponding irq, polling must be set up instead of simply ignoring the
>>> missing irq.
>>
>> I agree that none of the "return NULL" changes are necessary.
>>
>> However, note that it's not really possible to recover here by polling;
>> some of the gpios may have been claimed and some not when the irq
>> claim failure occurs.
>> How to know which gpios were claimed?
> 
> I don't understand this. You cannot recover if a GPIO is used by a
> different driver/device. If a given GPIO doesn't support triggering an
> irq, polling can help, it must of course be implemented correctly.
> And if gpio_mctrl implements this (which is the right thing to do, as it
> is independant from the actual driver) all necessary knowledge is there.

Ah, sorry.

I thought the both the code comment and your review comment above
about "polling must be set up" implied the caller could do something
about that, which obviously the caller can't.

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König May 24, 2016, 3:05 p.m. UTC | #8

On Tue, May 24, 2016 at 07:40:51AM -0700, Peter Hurley wrote:
> On 05/24/2016 01:09 AM, Uwe Kleine-König wrote:
> > On Mon, May 23, 2016 at 11:02:53AM -0700, Peter Hurley wrote:
> >> On 05/23/2016 02:09 AM, Uwe Kleine-König wrote:
> >> However, note that it's not really possible to recover here by polling;
> >> some of the gpios may have been claimed and some not when the irq
> >> claim failure occurs.
> >> How to know which gpios were claimed?
> > 
> > I don't understand this. You cannot recover if a GPIO is used by a
> > different driver/device. If a given GPIO doesn't support triggering an
> > irq, polling can help, it must of course be implemented correctly.
> > And if gpio_mctrl implements this (which is the right thing to do, as it
> > is independant from the actual driver) all necessary knowledge is there.
> 
> Ah, sorry.
> 
> I thought the both the code comment and your review comment above
> about "polling must be set up" implied the caller could do something
> about that, which obviously the caller can't.

ack.

Uwe

Patch

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index a868595..35588c5 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -52,6 +52,9 @@  void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
 	int value_array[UART_GPIO_MAX];
 	unsigned int count = 0;
 
+	if (gpios == NULL)
+		return;
+
 	for (i = 0; i < UART_GPIO_MAX; i++)
 		if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
 			desc_array[count] = gpios->gpio[i];
@@ -73,6 +76,9 @@  unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
 {
 	enum mctrl_gpio_idx i;
 
+	if (gpios == NULL)
+		return *mctrl;
+
 	for (i = 0; i < UART_GPIO_MAX; i++) {
 		if (gpios->gpio[i] && !mctrl_gpios_desc[i].dir_out) {
 			if (gpiod_get_value(gpios->gpio[i]))
@@ -91,6 +97,9 @@  mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl)
 {
 	enum mctrl_gpio_idx i;
 
+	if (gpios == NULL)
+		return *mctrl;
+
 	for (i = 0; i < UART_GPIO_MAX; i++) {
 		if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
 			if (gpiod_get_value(gpios->gpio[i]))
@@ -178,7 +187,7 @@  struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
 
 	gpios = mctrl_gpio_init_noauto(port->dev, idx);
 	if (IS_ERR(gpios))
-		return gpios;
+		return NULL;
 
 	gpios->port = port;
 
@@ -193,7 +202,7 @@  struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
 			dev_err(port->dev,
 				"failed to find corresponding irq for %s (idx=%d, err=%d)\n",
 				mctrl_gpios_desc[i].name, idx, ret);
-			return ERR_PTR(ret);
+			return NULL;
 		}
 		gpios->irq[i] = ret;
 
@@ -209,7 +218,7 @@  struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
 			dev_err(port->dev,
 				"failed to request irq for %s (idx=%d, err=%d)\n",
 				mctrl_gpios_desc[i].name, idx, ret);
-			return ERR_PTR(ret);
+			return NULL;
 		}
 	}
 
@@ -221,6 +230,9 @@  void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
 {
 	enum mctrl_gpio_idx i;
 
+	if (gpios == NULL)
+		return;
+
 	for (i = 0; i < UART_GPIO_MAX; i++) {
 		if (gpios->irq[i])
 			devm_free_irq(gpios->port->dev, gpios->irq[i], gpios);
@@ -236,6 +248,9 @@  void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios)
 {
 	enum mctrl_gpio_idx i;
 
+	if (gpios == NULL)
+		return;
+
 	/* .enable_ms may be called multiple times */
 	if (gpios->mctrl_on)
 		return;
@@ -258,6 +273,9 @@  void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
 {
 	enum mctrl_gpio_idx i;
 
+	if (gpios == NULL)
+		return;
+
 	if (!gpios->mctrl_on)
 		return;
 
diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
index fa000bc..82c5fbf 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.h
+++ b/drivers/tty/serial/serial_mctrl_gpio.h
@@ -130,7 +130,7 @@  struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
 static inline
 struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline