Re: [PATCH] tty: n_gsm: Don't block input queue by waiting MSC

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Aug 25, 2025 at 04:55:00PM +0300, Seppo Takalo wrote:
> Add parameter "wait" for gsm_modem_update() to indicate if we
> should wait for the response.
> 
> Currently gsm_queue() processes incoming frames and when opening
> a DLC channel it calls gsm_dlci_open() which calls gsm_modem_update().
> If basic mode is used it calls gsm_modem_upd_via_msc() and it
> cannot block the input queue by waiting the response to come
> into the same input queue.
> 
> Instead allow sending Modem Status Command without waiting for remote
> end to respond.
> 
> Signed-off-by: Seppo Takalo <seppo.takalo@nordicsemi.no>

What commit id does this fix?

> ---
>  drivers/tty/n_gsm.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 8dd3f23af3d2..8e8475d9fbeb 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -454,7 +454,7 @@ static const u8 gsm_fcs8[256] = {
>  
>  static void gsm_dlci_close(struct gsm_dlci *dlci);
>  static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
> -static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
> +static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk, bool wait);

Adding a random boolean to a function is almost never a good idea.  Now
every time you call this function, you have to go and look up what that
boolean means.

Please never do that, instead make a "wrapper" function that will then
call this "core" function with the boolean set properly.  That way you
can name the wrapper functions in a way that describes what it does.

>  static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
>  								u8 ctrl);
>  static int gsm_send_packet(struct gsm_mux *gsm, struct gsm_msg *msg);
> @@ -2174,7 +2174,7 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
>  		pr_debug("DLCI %d goes open.\n", dlci->addr);
>  	/* Send current modem state */
>  	if (dlci->addr) {
> -		gsm_modem_update(dlci, 0);
> +		gsm_modem_update(dlci, 0, false);

See, what does false mean?  No clue :(

Why not call gsm_modem_update_and_wait() instead?

thanks,

greg k-h