Hi Gwenn,

> Please review the following patch:
> 
> From 6e006bd522124d0e8a2f6075099a21f7051a426f Mon Sep 17 00:00:00 2001
> From: Gwenn Bourree <[email protected]>
> Date: Mon, 29 Jun 2015 17:26:01 +0200
> Subject: [PATCH] Mux n_gsm: Add a DLCI hangup state and callback

this is borked. Consider using git format-patch and git send-email.

> 
> Use of asynchronous hangup instead of vhangup.

In general it is useful to have a bit more explanation in your patch on what it 
does, why it does it and how.

> 
> Signed-off-by: Gwenn Bourree <[email protected]>
> Signed-off-by: Gwenn Bourree <[email protected]>

You do not need to have yourself twice here.

> Signed-off-by: Mustapha Ben Zoubeir <[email protected]>
> Signed-off-by: Nicolas LOUIS <[email protected]>
> Reviewed-by: Ravindran, Arun <[email protected]

Please make sure reviewed-by are correct. I would use first name last name 
<email>. And make sure to close the email with >

> 
> ---
> drivers/tty/n_gsm.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index d6e0ea0..762f555 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -135,6 +135,7 @@ struct gsm_dlci {
> #define DLCI_OPENING          1       /* Sending SABM not seen UA */
> #define DLCI_OPEN             2       /* SABM/UA complete */
> #define DLCI_CLOSING          3       /* Sending DISC not seen UA/DM */
> +#define DLCI_HANGUP          4       /*HANGUP received  */

Please follow coding style. The example of the comment is above.

>       struct mutex mutex;
> 
>       /* Link layer */
> @@ -1530,7 +1531,8 @@ static void gsm_dlci_begin_open(struct gsm_dlci
> *dlci)
> static void gsm_dlci_begin_close(struct gsm_dlci *dlci)
> {
>       struct gsm_mux *gsm = dlci->gsm;
> -     if (dlci->state == DLCI_CLOSED || dlci->state == DLCI_CLOSING)
> +     if (dlci->state == DLCI_CLOSED || dlci->state == DLCI_CLOSING ||
> +             dlci->state == DLCI_HANGUP)

I am pretty sure this indentation is wrong. You can not tell the actual code 
block apart from the condition. So either align with the dlci->state above or 
use two tabs. Check what coding style is common in this code and choose the one 
used in similar multiline if conditions.

>               return;
>       dlci->retries = gsm->n2;
>       dlci->state = DLCI_CLOSING;
> @@ -1717,7 +1719,7 @@ static void gsm_dlci_release(struct gsm_dlci
> *dlci)
>               gsm_destroy_network(dlci);
>               mutex_unlock(&dlci->mutex);
> 
> -             tty_vhangup(tty);
> +             tty_hangup(tty);
> 
>               tty_port_tty_set(&dlci->port, NULL);
>               tty_kref_put(tty);
> @@ -2339,6 +2341,26 @@ static void gsmld_flush_buffer(struct tty_struct
> *tty)
> }
> 
> /**
> + *   gsmld_hangup            -       hangup the ldisc for this tty
> + *   @tty: device
> + */
> +
> +static int gsmld_hangup(struct tty_struct *tty)
> +{
> +     struct gsm_mux *gsm = tty->disc_data;
> +     int i;
> +     struct gsm_dlci *dlci;
> +
> +     for (i = NUM_DLCI-1; i >= 0; i--) {
> +             dlci = gsm->dlci[i];

I would have moved the struct gsm_dlci declaration into the body of the for 
loop.

> +             if (dlci)
> +                     dlci->state = DLCI_HANGUP;
> +     }
> +
> +     return 0;
> +}
> +
> +/**
>  *    gsmld_close             -       close the ldisc for this tty
>  *    @tty: device
>  *
> @@ -2836,6 +2858,7 @@ static struct tty_ldisc_ops tty_ldisc_packet = {
>       .name            = "n_gsm",
>       .open            = gsmld_open,
>       .close           = gsmld_close,
> +     .hangup          = gsmld_hangup,
>       .flush_buffer    = gsmld_flush_buffer,
>       .chars_in_buffer = gsmld_chars_in_buffer,
>       .read            = gsmld_read,

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to