[PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[Aastha Gupta]

This patch fixes following checkpatch.pl checks:
CHECK: struct mutex definition without comment

Signed-off-by: Aastha Gupta <aastha.gupta4104@gmail.com>
---
 drivers/staging/most/hdm-usb/hdm_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
index 85775da..9c58598 100644
--- a/drivers/staging/most/hdm-usb/hdm_usb.c
+++ b/drivers/staging/most/hdm-usb/hdm_usb.c
@@ -122,7 +122,7 @@ struct most_dev {
 	bool is_channel_healthy[MAX_NUM_ENDPOINTS];
 	struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
 	struct usb_anchor *busy_urbs;
-	struct mutex io_mutex;
+	struct mutex io_mutex; /* synchronize I/O with disconnect */
 	struct timer_list link_stat_timer;
 	struct work_struct poll_work_obj;
 	void (*on_netinfo)(struct most_interface *, unsigned char,
-- 
2.7.4

Re: [Outreachy kernel] [PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[Julia Lawall]

On Sat, 16 Sep 2017, Aastha Gupta wrote:

> This patch fixes following checkpatch.pl checks:
> CHECK: struct mutex definition without comment
>
> Signed-off-by: Aastha Gupta <aastha.gupta4104@gmail.com>
> ---
>  drivers/staging/most/hdm-usb/hdm_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
> index 85775da..9c58598 100644
> --- a/drivers/staging/most/hdm-usb/hdm_usb.c
> +++ b/drivers/staging/most/hdm-usb/hdm_usb.c
> @@ -122,7 +122,7 @@ struct most_dev {
>  	bool is_channel_healthy[MAX_NUM_ENDPOINTS];
>  	struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
>  	struct usb_anchor *busy_urbs;
> -	struct mutex io_mutex;
> +	struct mutex io_mutex; /* synchronize I/O with disconnect */

Why did you choose this comment?

julia

>  	struct timer_list link_stat_timer;
>  	struct work_struct poll_work_obj;
>  	void (*on_netinfo)(struct most_interface *, unsigned char,
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1505544199-13800-1-git-send-email-aastha.gupta4104%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[Aastha Gupta]

On Sat, Sep 16, 2017 at 1:18 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sat, 16 Sep 2017, Aastha Gupta wrote:
>
>> This patch fixes following checkpatch.pl checks:
>> CHECK: struct mutex definition without comment
>>
>> Signed-off-by: Aastha Gupta <aastha.gupta4104@gmail.com>
>> ---
>>  drivers/staging/most/hdm-usb/hdm_usb.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
>> index 85775da..9c58598 100644
>> --- a/drivers/staging/most/hdm-usb/hdm_usb.c
>> +++ b/drivers/staging/most/hdm-usb/hdm_usb.c
>> @@ -122,7 +122,7 @@ struct most_dev {
>>       bool is_channel_healthy[MAX_NUM_ENDPOINTS];
>>       struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
>>       struct usb_anchor *busy_urbs;
>> -     struct mutex io_mutex;
>> +     struct mutex io_mutex; /* synchronize I/O with disconnect */
>
> Why did you choose this comment?
>
> julia

The use of this struct mutex was mentioned before in the code.

Aastha
>
>>       struct timer_list link_stat_timer;
>>       struct work_struct poll_work_obj;
>>       void (*on_netinfo)(struct most_interface *, unsigned char,
>> --
>> 2.7.4
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1505544199-13800-1-git-send-email-aastha.gupta4104%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>>

Re: [Outreachy kernel] [PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[Julia Lawall]

On Sat, 16 Sep 2017, Aastha Gupta wrote:

> On Sat, Sep 16, 2017 at 1:18 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Sat, 16 Sep 2017, Aastha Gupta wrote:
> >
> >> This patch fixes following checkpatch.pl checks:
> >> CHECK: struct mutex definition without comment
> >>
> >> Signed-off-by: Aastha Gupta <aastha.gupta4104@gmail.com>
> >> ---
> >>  drivers/staging/most/hdm-usb/hdm_usb.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
> >> index 85775da..9c58598 100644
> >> --- a/drivers/staging/most/hdm-usb/hdm_usb.c
> >> +++ b/drivers/staging/most/hdm-usb/hdm_usb.c
> >> @@ -122,7 +122,7 @@ struct most_dev {
> >>       bool is_channel_healthy[MAX_NUM_ENDPOINTS];
> >>       struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
> >>       struct usb_anchor *busy_urbs;
> >> -     struct mutex io_mutex;
> >> +     struct mutex io_mutex; /* synchronize I/O with disconnect */
> >
> > Why did you choose this comment?
> >
> > julia
>
> The use of this struct mutex was mentioned before in the code.

I see that it is just in the doc of the same structure.  I'm not sure that
a double comment is needed in this case. I wonder if checkpatch
should/could be extended to address this.

julia

>
> Aastha
> >
> >>       struct timer_list link_stat_timer;
> >>       struct work_struct poll_work_obj;
> >>       void (*on_netinfo)(struct most_interface *, unsigned char,
> >> --
> >> 2.7.4
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> >> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1505544199-13800-1-git-send-email-aastha.gupta4104%40gmail.com.
> >> For more options, visit https://groups.google.com/d/optout.
> >>
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAHXiS5-uqsTyjXv3KjnGnL9T7hKfEnMPL%2BRW-CXQE%2BU-oDFq2Q%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[Joe Perches]

On Sat, 2017-09-16 at 10:23 +0200, Julia Lawall wrote:
> On Sat, 16 Sep 2017, Aastha Gupta wrote:
> > On Sat, Sep 16, 2017 at 1:18 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > > On Sat, 16 Sep 2017, Aastha Gupta wrote:
> > > 
> > > > This patch fixes following checkpatch.pl checks:
> > > > CHECK: struct mutex definition without comment
[]
> > > > diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
[]
> > > > @@ -122,7 +122,7 @@ struct most_dev {
> > > >       bool is_channel_healthy[MAX_NUM_ENDPOINTS];
> > > >       struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
> > > >       struct usb_anchor *busy_urbs;
> > > > -     struct mutex io_mutex;
> > > > +     struct mutex io_mutex; /* synchronize I/O with disconnect */
> > > 
> > > Why did you choose this comment?
> > > 
> > > julia
> > 
> > The use of this struct mutex was mentioned before in the code.
> 
> I see that it is just in the doc of the same structure.  I'm not sure that
> a double comment is needed in this case. I wonder if checkpatch
> should/could be extended to address this.

I think that's not feasible.

checkpatch is a patch context based check and wouldn't
necessarily know that the mutex is documented elsewhere.

Re: [Outreachy kernel] [PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[Julia Lawall]

On Sat, 16 Sep 2017, Joe Perches wrote:

> On Sat, 2017-09-16 at 10:23 +0200, Julia Lawall wrote:
> > On Sat, 16 Sep 2017, Aastha Gupta wrote:
> > > On Sat, Sep 16, 2017 at 1:18 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > > > On Sat, 16 Sep 2017, Aastha Gupta wrote:
> > > >
> > > > > This patch fixes following checkpatch.pl checks:
> > > > > CHECK: struct mutex definition without comment
> []
> > > > > diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
> []
> > > > > @@ -122,7 +122,7 @@ struct most_dev {
> > > > >       bool is_channel_healthy[MAX_NUM_ENDPOINTS];
> > > > >       struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
> > > > >       struct usb_anchor *busy_urbs;
> > > > > -     struct mutex io_mutex;
> > > > > +     struct mutex io_mutex; /* synchronize I/O with disconnect */
> > > >
> > > > Why did you choose this comment?
> > > >
> > > > julia
> > >
> > > The use of this struct mutex was mentioned before in the code.
> >
> > I see that it is just in the doc of the same structure.  I'm not sure that
> > a double comment is needed in this case. I wonder if checkpatch
> > should/could be extended to address this.
>
> I think that's not feasible.
>
> checkpatch is a patch context based check and wouldn't
> necessarily know that the mutex is documented elsewhere.

OK.  It doesn't keep any history?  The kerneldoc is before the structure
field declaration.

julia

Re: [Outreachy kernel] [PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[Joe Perches]

On Sat, 2017-09-16 at 11:35 +0200, Julia Lawall wrote:
> 
> On Sat, 16 Sep 2017, Joe Perches wrote:
> 
> > On Sat, 2017-09-16 at 10:23 +0200, Julia Lawall wrote:
> > > On Sat, 16 Sep 2017, Aastha Gupta wrote:
> > > > On Sat, Sep 16, 2017 at 1:18 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > > > > On Sat, 16 Sep 2017, Aastha Gupta wrote:
> > > > > 
> > > > > > This patch fixes following checkpatch.pl checks:
> > > > > > CHECK: struct mutex definition without comment
> > 
> > []
> > > > > > diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
> > 
> > []
> > > > > > @@ -122,7 +122,7 @@ struct most_dev {
> > > > > >       bool is_channel_healthy[MAX_NUM_ENDPOINTS];
> > > > > >       struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
> > > > > >       struct usb_anchor *busy_urbs;
> > > > > > -     struct mutex io_mutex;
> > > > > > +     struct mutex io_mutex; /* synchronize I/O with disconnect */
> > > > > 
> > > > > Why did you choose this comment?
> > > > > 
> > > > > julia
> > > > 
> > > > The use of this struct mutex was mentioned before in the code.
> > > 
> > > I see that it is just in the doc of the same structure.  I'm not sure that
> > > a double comment is needed in this case. I wonder if checkpatch
> > > should/could be extended to address this.
> > 
> > I think that's not feasible.
> > 
> > checkpatch is a patch context based check and wouldn't
> > necessarily know that the mutex is documented elsewhere.
> 
> OK.  It doesn't keep any history?  The kerneldoc is before the structure
> field declaration.

checkpatch reads and stores its input.

checkpatch is, with very few exceptions, a line based
regex scanner.

An input patch context is typically 3 lines above
and below modified content.

Whatever context outside of that +/- 3 is unavailable.

Kernel-doc comments are not parsed by checkpatch as
anything special.

checkpatch code that looks for specific comments looks
only at whether or not a particular block has a comment
at all, not the specific content of the comment.

The checkpatch function test is ctx_has_comment

You are welcome to try to expand the code but I think
there are better possible tools for this.

Re: [Outreachy kernel] [PATCH] staging: most: hdm-usb: add comment for struct mutex definition

[Julia Lawall]

On Sat, 16 Sep 2017, Joe Perches wrote:

> On Sat, 2017-09-16 at 11:35 +0200, Julia Lawall wrote:
> >
> > On Sat, 16 Sep 2017, Joe Perches wrote:
> >
> > > On Sat, 2017-09-16 at 10:23 +0200, Julia Lawall wrote:
> > > > On Sat, 16 Sep 2017, Aastha Gupta wrote:
> > > > > On Sat, Sep 16, 2017 at 1:18 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > > > > > On Sat, 16 Sep 2017, Aastha Gupta wrote:
> > > > > >
> > > > > > > This patch fixes following checkpatch.pl checks:
> > > > > > > CHECK: struct mutex definition without comment
> > >
> > > []
> > > > > > > diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
> > >
> > > []
> > > > > > > @@ -122,7 +122,7 @@ struct most_dev {
> > > > > > >       bool is_channel_healthy[MAX_NUM_ENDPOINTS];
> > > > > > >       struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
> > > > > > >       struct usb_anchor *busy_urbs;
> > > > > > > -     struct mutex io_mutex;
> > > > > > > +     struct mutex io_mutex; /* synchronize I/O with disconnect */
> > > > > >
> > > > > > Why did you choose this comment?
> > > > > >
> > > > > > julia
> > > > >
> > > > > The use of this struct mutex was mentioned before in the code.
> > > >
> > > > I see that it is just in the doc of the same structure.  I'm not sure that
> > > > a double comment is needed in this case. I wonder if checkpatch
> > > > should/could be extended to address this.
> > >
> > > I think that's not feasible.
> > >
> > > checkpatch is a patch context based check and wouldn't
> > > necessarily know that the mutex is documented elsewhere.
> >
> > OK.  It doesn't keep any history?  The kerneldoc is before the structure
> > field declaration.
>
> checkpatch reads and stores its input.
>
> checkpatch is, with very few exceptions, a line based
> regex scanner.
>
> An input patch context is typically 3 lines above
> and below modified content.
>
> Whatever context outside of that +/- 3 is unavailable.
>
> Kernel-doc comments are not parsed by checkpatch as
> anything special.
>
> checkpatch code that looks for specific comments looks
> only at whether or not a particular block has a comment
> at all, not the specific content of the comment.
>
> The checkpatch function test is ctx_has_comment
>
> You are welcome to try to expand the code but I think
> there are better possible tools for this.

OK, thanks.

julia