[PATCH] n_gsm: Fix write handling for zero bytes written

[PATCH] n_gsm: Fix write handling for zero bytes written

[Tony Lindgren]

If write returns zero we currently end up removing the message
from the queue. Instead of removing the message, we want to just
break out of the loop just like we already do for error codes.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/n_gsm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -691,7 +691,8 @@ static void gsm_data_kick(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 			print_hex_dump_bytes("gsm_data_kick: ",
 					     DUMP_PREFIX_OFFSET,
 					     gsm->txframe, len);
-		if (gsm->output(gsm, gsm->txframe, len) < 0)
+
+		if (gsm->output(gsm, gsm->txframe, len) <= 0)
 			break;
 		/* FIXME: Can eliminate one SOF in many more cases */
 		gsm->tx_bytes -= msg->len;
-- 
2.28.0

Re: [PATCH] n_gsm: Fix write handling for zero bytes written

[Jiri Slaby]

On 17. 08. 20, 15:54, Tony Lindgren wrote:
> If write returns zero we currently end up removing the message
> from the queue. Instead of removing the message, we want to just
> break out of the loop just like we already do for error codes.

When exactly does the only writer (gsmld_output) return zero for
non-zero len parameter?

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/n_gsm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -691,7 +691,8 @@ static void gsm_data_kick(struct gsm_mux *gsm, struct gsm_dlci *dlci)
>  			print_hex_dump_bytes("gsm_data_kick: ",
>  					     DUMP_PREFIX_OFFSET,
>  					     gsm->txframe, len);
> -		if (gsm->output(gsm, gsm->txframe, len) < 0)
> +
> +		if (gsm->output(gsm, gsm->txframe, len) <= 0)
>  			break;
>  		/* FIXME: Can eliminate one SOF in many more cases */
>  		gsm->tx_bytes -= msg->len;
> 

thanks,
-- 
js

Re: [PATCH] n_gsm: Fix write handling for zero bytes written

[Tony Lindgren]

Hi,

* Jiri Slaby <jirislaby@kernel.org> [200818 08:24]:
> On 17. 08. 20, 15:54, Tony Lindgren wrote:
> > If write returns zero we currently end up removing the message
> > from the queue. Instead of removing the message, we want to just
> > break out of the loop just like we already do for error codes.
> 
> When exactly does the only writer (gsmld_output) return zero for
> non-zero len parameter?

I ran into this when testing with the WIP serial core PM runtime
changes from Andy Shevchenko earlier. If there are also other
cases where we have serial drivers return 0, I don't know about
them.

Basically with the WIP serial core changes, if the open serial port
is in PM runtime suspended state with it's autosuspend_delay_ms
expired, we have write return 0 and just wake up the serial device
on TX.

I don't think there's much anything else we can currently do there
in the PM runtime suspended case as we want to get rid of the
remaining pm_runtime_irq_safe() dependencies as it takes a permanent
usage count on the parent device.

Regards,

Tony

> 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  drivers/tty/n_gsm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -691,7 +691,8 @@ static void gsm_data_kick(struct gsm_mux *gsm, struct gsm_dlci *dlci)
> >  			print_hex_dump_bytes("gsm_data_kick: ",
> >  					     DUMP_PREFIX_OFFSET,
> >  					     gsm->txframe, len);
> > -		if (gsm->output(gsm, gsm->txframe, len) < 0)
> > +
> > +		if (gsm->output(gsm, gsm->txframe, len) <= 0)
> >  			break;
> >  		/* FIXME: Can eliminate one SOF in many more cases */
> >  		gsm->tx_bytes -= msg->len;
> > 
> 
> thanks,
> -- 
> js

Re: [PATCH] n_gsm: Fix write handling for zero bytes written

[Jiri Slaby]

On 18. 08. 20, 11:56, Tony Lindgren wrote:
> Hi,
> 
> * Jiri Slaby <jirislaby@kernel.org> [200818 08:24]:
>> On 17. 08. 20, 15:54, Tony Lindgren wrote:
>>> If write returns zero we currently end up removing the message
>>> from the queue. Instead of removing the message, we want to just
>>> break out of the loop just like we already do for error codes.
>>
>> When exactly does the only writer (gsmld_output) return zero for
>> non-zero len parameter?
> 
> I ran into this when testing with the WIP serial core PM runtime
> changes from Andy Shevchenko earlier. If there are also other
> cases where we have serial drivers return 0, I don't know about
> them.

Sorry, I don't understand: my gsmld_output() ignores the return value
from drivers' write and returns something greater than zero or a
negative error. What tree/SHA do you run?

> Basically with the WIP serial core changes, if the open serial port
> is in PM runtime suspended state with it's autosuspend_delay_ms
> expired, we have write return 0 and just wake up the serial device
> on TX.


-- 
js

Re: [PATCH] n_gsm: Fix write handling for zero bytes written

[Tony Lindgren]

* Jiri Slaby <jirislaby@kernel.org> [200818 10:14]:
> On 18. 08. 20, 11:56, Tony Lindgren wrote:
> > Hi,
> > 
> > * Jiri Slaby <jirislaby@kernel.org> [200818 08:24]:
> >> On 17. 08. 20, 15:54, Tony Lindgren wrote:
> >>> If write returns zero we currently end up removing the message
> >>> from the queue. Instead of removing the message, we want to just
> >>> break out of the loop just like we already do for error codes.
> >>
> >> When exactly does the only writer (gsmld_output) return zero for
> >> non-zero len parameter?
> > 
> > I ran into this when testing with the WIP serial core PM runtime
> > changes from Andy Shevchenko earlier. If there are also other
> > cases where we have serial drivers return 0, I don't know about
> > them.
> 
> Sorry, I don't understand: my gsmld_output() ignores the return value
> from drivers' write and returns something greater than zero or a
> negative error. What tree/SHA do you run?

Oh right, good catch. I also had my WIP serdev-ngsm patches applied
that uses gsm_serdev_output() and returns the bytes written. Andy's
patches do not touch n_gsm.c.

Hmm sounds like we should also start returning value also from
gsmld_output()? Any objections to making that change?

For reference, Andy's WIP serial cor PM runtime changes are at:

https://gitlab.com/andy-shev/next.git/ topic/uart/rpm-plus

Regards,

Tony

Re: [PATCH] n_gsm: Fix write handling for zero bytes written

[Jiri Slaby]

On 18. 08. 20, 12:47, Tony Lindgren wrote:
> * Jiri Slaby <jirislaby@kernel.org> [200818 10:14]:
>> On 18. 08. 20, 11:56, Tony Lindgren wrote:
>>> Hi,
>>>
>>> * Jiri Slaby <jirislaby@kernel.org> [200818 08:24]:
>>>> On 17. 08. 20, 15:54, Tony Lindgren wrote:
>>>>> If write returns zero we currently end up removing the message
>>>>> from the queue. Instead of removing the message, we want to just
>>>>> break out of the loop just like we already do for error codes.
>>>>
>>>> When exactly does the only writer (gsmld_output) return zero for
>>>> non-zero len parameter?
>>>
>>> I ran into this when testing with the WIP serial core PM runtime
>>> changes from Andy Shevchenko earlier. If there are also other
>>> cases where we have serial drivers return 0, I don't know about
>>> them.
>>
>> Sorry, I don't understand: my gsmld_output() ignores the return value
>> from drivers' write and returns something greater than zero or a
>> negative error. What tree/SHA do you run?
> 
> Oh right, good catch. I also had my WIP serdev-ngsm patches applied
> that uses gsm_serdev_output() and returns the bytes written. Andy's
> patches do not touch n_gsm.c.
> 
> Hmm sounds like we should also start returning value also from
> gsmld_output()? Any objections to making that change?

No objections here.

thanks,
-- 
js
suse labs

Re: [PATCH] n_gsm: Fix write handling for zero bytes written

[Tony Lindgren]

* Jiri Slaby <jirislaby@kernel.org> [200819 06:20]:
> On 18. 08. 20, 12:47, Tony Lindgren wrote:
> > * Jiri Slaby <jirislaby@kernel.org> [200818 10:14]:
> >> On 18. 08. 20, 11:56, Tony Lindgren wrote:
> >>> Hi,
> >>>
> >>> * Jiri Slaby <jirislaby@kernel.org> [200818 08:24]:
> >>>> On 17. 08. 20, 15:54, Tony Lindgren wrote:
> >>>>> If write returns zero we currently end up removing the message
> >>>>> from the queue. Instead of removing the message, we want to just
> >>>>> break out of the loop just like we already do for error codes.
> >>>>
> >>>> When exactly does the only writer (gsmld_output) return zero for
> >>>> non-zero len parameter?
> >>>
> >>> I ran into this when testing with the WIP serial core PM runtime
> >>> changes from Andy Shevchenko earlier. If there are also other
> >>> cases where we have serial drivers return 0, I don't know about
> >>> them.
> >>
> >> Sorry, I don't understand: my gsmld_output() ignores the return value
> >> from drivers' write and returns something greater than zero or a
> >> negative error. What tree/SHA do you run?
> > 
> > Oh right, good catch. I also had my WIP serdev-ngsm patches applied
> > that uses gsm_serdev_output() and returns the bytes written. Andy's
> > patches do not touch n_gsm.c.
> > 
> > Hmm sounds like we should also start returning value also from
> > gsmld_output()? Any objections to making that change?
> 
> No objections here.

OK thanks, I'll post an updated patch.

Regards,

Tony