Re: [PATCH] ring-buffer: Update "shortest_full" in polling

[Julia Lawall]

On Fri, 29 Sep 2023, Steven Rostedt wrote:

> From: "Steven Rostedt (Google)" 
>
> It was discovered that the ring buffer polling was incorrectly stating
> that read would not block, but that's because polling did not take into
> account that reads will block if the "buffer-percent" was set. Instead,
> the ring buffer polling would say reads would not block if there was any
> data in the ring buffer. This was incorrect behavior from a user space
> point of view. This was fixed by commit 42fb0a1e84ff by having the polling
> code check if the ring buffer had more data than what the user specified
> "buffer percent" had.
>
> The problem now is that the polling code did not register itself to the
> writer that it wanted to wait for a specific "full" value of the ring
> buffer. The result was that the writer would wake the polling waiter
> whenever there was a new event. The polling waiter would then wake up, see
> that there's not enough data in the ring buffer to notify user space and
> then go back to sleep. The next event would wake it up again.
>
> Before the polling fix was added, the code would wake up around 100 times
> for a hackbench 30 benchmark. After the "fix", due to the constant waking
> of the writer, it would wake up over 11, times! It would never leave
> the kernel, so the user space behavior was still "correct", but this
> definitely is not the desired effect.
>
> To fix this, have the polling code add what it's waiting for to the
> "shortest_full" variable, to tell the writer not to wake it up if the
> buffer is not as full as it expects to be.
>
> Note, after this fix, it appears that the waiter is now woken up around 2x
> the times it was before (~200). This is a tremendous improvement from the
> 11,000 times, but I will need to spend some time to see why polling is
> more aggressive in its wakeups than the read blocking code.

Actually, in my test it has gone from 276 wakeups in 6.0 to only 3 with
this patch.

I can do some more tests.

>
> Cc: sta...@vger.kernel.org
> Fixes: 42fb0a1e84ff ("tracing/ring-buffer: Have polling block on watermark")
> Reported-by: Julia Lawall 
> Signed-off-by: Steven Rostedt (Google) 

Tested-by: Julia Lawall 

julia

> ---
>  kernel/trace/ring_buffer.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 28daf0ce95c5..515cafdb18d9 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -1137,6 +1137,9 @@ __poll_t ring_buffer_poll_wait(struct trace_buffer 
> *buffer, int cpu,
>   if (full) {
>   poll_wait(filp, >full_waiters, poll_table);
>   work->full_waiters_pending = true;
> + if (!cpu_buffer->shortest_full ||
> + cpu_buffer->shortest_full > full)
> + cpu_buffer->shortest_full = full;
>   } else {
>   poll_wait(filp, >waiters, poll_table);
>   work->waiters_pending = true;
> --
> 2.40.1
>
>

Re: [PATCH] tracing/eprobe: drop unneeded breaks

[Julia Lawall]

On Fri, 29 Sep 2023, Masami Hiramatsu  wrote:

> On Thu, 28 Sep 2023 12:43:34 +0200
> Julia Lawall  wrote:
>
> > Drop break after return.
> >
>
> Good catch! This looks good to me.
>
> Acked-by: Masami Hiramatsu (Google) 
>
> And
>
> Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")

Thanks.  I didn't include that because it's not a bug.  But it does break
Coccinelle, which is how I noticed it.

julia

>
> > Signed-off-by: Julia Lawall 
> >
> > ---
> >  kernel/trace/trace_eprobe.c |5 +
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> > index 72714cbf475c..03c851f57969 100644
> > --- a/kernel/trace/trace_eprobe.c
> > +++ b/kernel/trace/trace_eprobe.c
> > @@ -788,12 +788,9 @@ find_and_get_event(const char *system, const char 
> > *event_name)
> > name = trace_event_name(tp_event);
> > if (!name || strcmp(event_name, name))
> > continue;
> > -   if (!trace_event_try_get_ref(tp_event)) {
> > +   if (!trace_event_try_get_ref(tp_event))
> > return NULL;
> > -   break;
> > -   }
> > return tp_event;
> > -   break;
> > }
> > return NULL;
> >  }
> >
>
>
> --
> Masami Hiramatsu (Google) 
>

[PATCH] tracing/eprobe: drop unneeded breaks

[Julia Lawall]

Drop break after return.

Signed-off-by: Julia Lawall 

---
 kernel/trace/trace_eprobe.c |5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 72714cbf475c..03c851f57969 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -788,12 +788,9 @@ find_and_get_event(const char *system, const char 
*event_name)
name = trace_event_name(tp_event);
if (!name || strcmp(event_name, name))
continue;
-   if (!trace_event_try_get_ref(tp_event)) {
+   if (!trace_event_try_get_ref(tp_event))
return NULL;
-   break;
-   }
return tp_event;
-   break;
}
return NULL;
 }

[PATCH] Coccinelle: drop context *s

[Julia Lawall]

Context mode is not supported, so the *s are just confusing to people
who use the rule outside of make coccicheck.  So, drop the *s.

Fixes: 6dd9379e8f32 ("coccinelle: also catch kzfree() issues")
Reported-by: Fabio M. De Francesco 
Signed-off-by: Julia Lawall 

---
 scripts/coccinelle/free/kfree.cocci |   12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/coccinelle/free/kfree.cocci 
b/scripts/coccinelle/free/kfree.cocci
index 168568386034..9b6e2037c2a9 100644
--- a/scripts/coccinelle/free/kfree.cocci
+++ b/scripts/coccinelle/free/kfree.cocci
@@ -22,9 +22,9 @@ position p1;
 @@
 
 (
-* kfree@p1(E)
+ kfree@p1(E)
 |
-* kfree_sensitive@p1(E)
+ kfree_sensitive@p1(E)
 )
 
 @print expression@
@@ -66,9 +66,9 @@ position ok;
 
 while (1) { ...
 (
-* kfree@ok(E)
+ kfree@ok(E)
 |
-* kfree_sensitive@ok(E)
+ kfree_sensitive@ok(E)
 )
   ... when != break;
   when != goto l;
@@ -84,9 +84,9 @@ position free.p1!=loop.ok,p2!={print.p,sz.p};
 @@
 
 (
-* kfree@p1(E,...)
+ kfree@p1(E,...)
 |
-* kfree_sensitive@p1(E,...)
+ kfree_sensitive@p1(E,...)
 )
 ...
 (

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Julia Lawall]

On Fri, 16 Apr 2021, Sakari Ailus wrote:

> On Fri, Apr 16, 2021 at 10:46:54AM +0200, Julia Lawall wrote:
> > > > If you're running into the 80 character limit, then it's fine to use
> > > > two tabs.  I think we have been rejecting patches that push align the
> > > > parameters but push past the 80 character limit.  Using one tab is
> > > > confusing because it makes the decalarations line up with the code.
> > >
> > > Interesting. Do you have an example of this? I've thought checkpatch.pl
> > > gave a warning if the line ended with an opening parenthesis no matter
> > > what.
> >
> > Checkpatch is a perl script that searches for patterns that often indicate
> > code that is hard to understand.  It is not a precise definition of what
> > is allowed in the Linux kernel.  Humans have to amke compromises.
>
> Yeah... but I'd think if this is a preferred style then the warning could
> be omitted. It might not be that difficult.

No idea.  It involves looking at two successive lines, which may make it
more complicated.  Probably the biggest problem would be knowing whether
the line being looked at represents a function header.  Maybe that could
be detected by the fact that there is normally no space at the beginning
of the line?

julia

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Julia Lawall]

> > If you're running into the 80 character limit, then it's fine to use
> > two tabs.  I think we have been rejecting patches that push align the
> > parameters but push past the 80 character limit.  Using one tab is
> > confusing because it makes the decalarations line up with the code.
>
> Interesting. Do you have an example of this? I've thought checkpatch.pl
> gave a warning if the line ended with an opening parenthesis no matter
> what.

Checkpatch is a perl script that searches for patterns that often indicate
code that is hard to understand.  It is not a precise definition of what
is allowed in the Linux kernel.  Humans have to amke compromises.

julia

Re: [Outreachy kernel] Re: [PATCH v4 2/2] staging: media: zoran: add BIT() macro and align code

[Julia Lawall]

> > > +#define ZR36057_JMC_JPG_EXP_MODE (0 << 29)
> > > +#define ZR36057_JMC_JPG_CMP_MODE BIT(29)
> > > +#define ZR36057_JMC_MJPG_EXP_MODE(2 << 29)
> > > +#define ZR36057_JMC_MJPG_CMP_MODE(3 << 29)
> > Same as above. Please change back ZR36057_JMC_JPG_CMP_MODE to be (1 << 29).
> > Then this 2 bit field is consistent.
> >
> Sir, I guess this BIT(29) was already present. I didn't changed this.
> I will change this as you said.

It comes from this patch:

5e195bbddabdd94b15eeb60439cece996d58b329

Probably putting it back should be a different patch in the series.

julia

Re: [Outreachy patch] [PATCH v3 1/2] staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Wed, 14 Apr 2021, Fabio M. De Francesco wrote:

> Removed useless led_blink_hdl() prototype and definition. In wlancmds[]
> the slot #60 is now set to NULL using the macro GEN_MLME_EXT_HANDLER. This
> change has not unwanted side effects because the code in rtw_cmd.c checks
> if the function pointer is valid before using it.

When you send the series again, you can change not to no in the above.

julia

Re: [Outreachy kernel] [PATCH v3 2/2] staging: rtl8723bs: Remove everything related with LedBlink

[Julia Lawall]

On Wed, 14 Apr 2021, Fabio M. De Francesco wrote:

> Removed struct LedBlink_param. Removed LedBlink entries in
> rtw_cmd_callback[] and in wlancmds[]. Everything related to LedBlink is
> not anymore needed. Removed extra blank lines in the two mentioned
> arrays and changend the numbers set in comments for having them in line
> with the shift.

It would be better not to remove the blank lines at the same time.  That
could be in another patch.  It is distracting here.

julia

>
> Reported-by: Fabio Aiuto 
> Reported-by: Greg Kroah-Hartman 
> Suggested-by: Dan Carpenter 
> Signed-off-by: Fabio M. De Francesco 
> ---
>
> Changes from v2: Added this patch as 2/2.
> Changes from v1: No changes.
>
>  drivers/staging/rtl8723bs/core/rtw_cmd.c| 27 ++---
>  drivers/staging/rtl8723bs/include/rtw_cmd.h | 14 +++
>  2 files changed, 11 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index f82dbd4f4c3d..a74e6846f2df 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -22,7 +22,6 @@ static struct _cmd_callback rtw_cmd_callback[] = {
>   {GEN_CMD_CODE(_Write_EEPROM), NULL},
>   {GEN_CMD_CODE(_Read_EFUSE), NULL},
>   {GEN_CMD_CODE(_Write_EFUSE), NULL},
> -
>   {GEN_CMD_CODE(_Read_CAM),   NULL},  /*10*/
>   {GEN_CMD_CODE(_Write_CAM),   NULL},
>   {GEN_CMD_CODE(_setBCNITV), NULL},
> @@ -33,7 +32,6 @@ static struct _cmd_callback rtw_cmd_callback[] = {
>   {GEN_CMD_CODE(_SetOpMode), NULL},
>   {GEN_CMD_CODE(_SiteSurvey), _survey_cmd_callback}, /*18*/
>   {GEN_CMD_CODE(_SetAuth), NULL},
> -
>   {GEN_CMD_CODE(_SetKey), NULL},  /*20*/
>   {GEN_CMD_CODE(_SetStaKey), _setstaKey_cmdrsp_callback},
>   {GEN_CMD_CODE(_SetAssocSta), _setassocsta_cmdrsp_callback},
> @@ -44,7 +42,6 @@ static struct _cmd_callback rtw_cmd_callback[] = {
>   {GEN_CMD_CODE(_SetDataRate), NULL},
>   {GEN_CMD_CODE(_GetDataRate), NULL},
>   {GEN_CMD_CODE(_SetPhyInfo), NULL},
> -
>   {GEN_CMD_CODE(_GetPhyInfo), NULL}, /*30*/
>   {GEN_CMD_CODE(_SetPhy), NULL},
>   {GEN_CMD_CODE(_GetPhy), NULL},
> @@ -55,7 +52,6 @@ static struct _cmd_callback rtw_cmd_callback[] = {
>   {GEN_CMD_CODE(_JoinbssRpt), NULL},
>   {GEN_CMD_CODE(_SetRaTable), NULL},
>   {GEN_CMD_CODE(_GetRaTable), NULL},
> -
>   {GEN_CMD_CODE(_GetCCXReport), NULL}, /*40*/
>   {GEN_CMD_CODE(_GetDTMReport),   NULL},
>   {GEN_CMD_CODE(_GetTXRateStatistics), NULL},
> @@ -67,24 +63,19 @@ static struct _cmd_callback rtw_cmd_callback[] = {
>   {GEN_CMD_CODE(_SwitchAntenna), NULL},
>   {GEN_CMD_CODE(_SetCrystalCap), NULL},
>   {GEN_CMD_CODE(_SetSingleCarrierTx), NULL},  /*50*/
> -
>   {GEN_CMD_CODE(_SetSingleToneTx), NULL}, /*51*/
>   {GEN_CMD_CODE(_SetCarrierSuppressionTx), NULL},
>   {GEN_CMD_CODE(_SetContinuousTx), NULL},
>   {GEN_CMD_CODE(_SwitchBandwidth), NULL}, /*54*/
>   {GEN_CMD_CODE(_TX_Beacon), NULL},/*55*/
> -
>   {GEN_CMD_CODE(_Set_MLME_EVT), NULL},/*56*/
>   {GEN_CMD_CODE(_Set_Drv_Extra), NULL},/*57*/
>   {GEN_CMD_CODE(_Set_H2C_MSG), NULL},/*58*/
>   {GEN_CMD_CODE(_SetChannelPlan), NULL},/*59*/
> - {GEN_CMD_CODE(_LedBlink), NULL},/*60*/
> -
> - {GEN_CMD_CODE(_SetChannelSwitch), NULL},/*61*/
> - {GEN_CMD_CODE(_TDLS), NULL},/*62*/
> - {GEN_CMD_CODE(_ChkBMCSleepq), NULL}, /*63*/
> -
> - {GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
> + {GEN_CMD_CODE(_SetChannelSwitch), NULL},/*60*/
> + {GEN_CMD_CODE(_TDLS), NULL},/*61*/
> + {GEN_CMD_CODE(_ChkBMCSleepq), NULL}, /*62*/
> + {GEN_CMD_CODE(_RunInThreadCMD), NULL},/*63*/
>  };
>
>  static struct cmd_hdl wlancmds[] = {
> @@ -144,17 +135,13 @@ static struct cmd_hdl wlancmds[] = {
>   GEN_MLME_EXT_HANDLER(0, NULL)
>   GEN_MLME_EXT_HANDLER(0, NULL)
>   GEN_MLME_EXT_HANDLER(sizeof(struct Tx_Beacon_param), tx_beacon_hdl) 
> /*55*/
> -
>   GEN_MLME_EXT_HANDLER(0, mlme_evt_hdl) /*56*/
>   GEN_MLME_EXT_HANDLER(0, rtw_drvextra_cmd_hdl) /*57*/
> -
>   GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
>   GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param), 
> set_chplan_hdl) /*59*/
> - GEN_MLME_EXT_HANDLER(0, NULL) /*60*/
> -
> - GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelSwitch_param), 
> set_csa_hdl) /*61*/
> - GEN_MLME_EXT_HANDLER(sizeof(struct TDLSoption_param), tdls_hdl) /*62*/
> - GEN_MLME_EXT_HANDLER(0, chk_bmc_sleepq_hdl) /*63*/
> + GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelSwitch_param), 
> set_csa_hdl) /*60*/
> + GEN_MLME_EXT_HANDLER(sizeof(struct TDLSoption_param), tdls_hdl) /*61*/
> + GEN_MLME_EXT_HANDLER(0, chk_bmc_sleepq_hdl) /*62*/
>   GEN_MLME_EXT_HANDLER(sizeof(struct RunInThread_param), 
> run_in_thread_hdl) /*63*/
>  };
>
> diff --git a/drivers/staging/rtl8723bs/include/rtw_cmd.h 
> 

Re: [Outreachy kernel] Re: [PATCH 1/2] staging: media: atomisp: pci: Correct identation in block of conditional statements in file atomisp_v4l2.c

[Julia Lawall]

On Wed, 14 Apr 2021, Dan Carpenter wrote:

> On Wed, Apr 14, 2021 at 11:06:02AM -0300, Aline Santana Cordeiro wrote:
> > Correct identation in block of conditional statements.
> > The function "v4l2_device_unregister_subdev()" depends on
> > the results of the macro function "list_for_each_entry_safe()".
> >
> > Signed-off-by: Aline Santana Cordeiro 
> > ---
> >  drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c 
> > b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > index 0295e2e..6d853f4 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > @@ -1178,7 +1178,7 @@ static void atomisp_unregister_entities(struct 
> > atomisp_device *isp)
> > atomisp_mipi_csi2_unregister_entities(>csi2_port[i]);
> >
> > list_for_each_entry_safe(sd, next, >v4l2_dev.subdevs, list)
> > -   v4l2_device_unregister_subdev(sd);
> > +   v4l2_device_unregister_subdev(sd);
> >
>
> It's really more common to use curly braces for list_for_each() one
> liners.
>
>   list_for_each_entry_safe(sd, next, >v4l2_dev.subdevs, list) {
>   v4l2_device_unregister_subdev(sd);
>   }

A quick test with grep shows 4000 lines containing list_for_each that
contain no {, out of 15000 lines containing list_for_each in all.

julia


>
> regards,
> dan carpenter
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210414143011.GV6021%40kadam.
>

Re: [Outreachy kernel] Re: [PATCH v3 4/4] staging: media: intel-ipu3: remove space before tabs

[Julia Lawall]

On Wed, 14 Apr 2021, Mitali Borkar wrote:

> On Tue, Apr 13, 2021 at 09:17:12PM +0300, Dan Carpenter wrote:
> > On Tue, Apr 13, 2021 at 08:59:34PM +0530, Mitali Borkar wrote:
> > > Removed unnecessary space before tabs to adhere to linux kernel coding
> > > style.
> > > Reported by checkpatch.
> > >
> > > Signed-off-by: Mitali Borkar 
> > > ---
> > >
> > > Changes from v2:- No changes.
> > > Changes from v1:- No changes.
> > >
> > >  drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h 
> > > b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > index 47e98979683c..42edac5ee4e4 100644
> > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > @@ -633,7 +633,7 @@ struct 
> > > ipu3_uapi_bnr_static_config_wb_gains_thr_config {
> > >   * @cg:  Gain coefficient for threshold calculation, [0, 31], default 8.
> > >   * @ci:  Intensity coefficient for threshold calculation. range [0, 0x1f]
> > >   *   default 6.
> > > - *   format: u3.2 (3 most significant bits represent whole number,
> > > + *format: u3.2 (3 most significant bits represent whole number,
> > >   *   2 least significant bits represent the fractional part
> >
> > Just remove the spaces, don't remove the tab.  It's looks silly now.
> >
> Okay Sir, do I have to send a v4 patch on this now?

Yes.  If you get feedback on a patch, you should send a new version.

julia

>
> > regards,
> > dan carpenter
> >
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHX3iVCNXJlOsmuQ%40kali.
>

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> On Tuesday, April 13, 2021 8:57:20 PM CEST Julia Lawall wrote:
> > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 8:20:50 PM CEST Dan Carpenter wrote:
> > > > On Tue, Apr 13, 2021 at 06:47:06PM +0200, Fabio M. De Francesco
> wrote:
> > > > > On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > > > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > > > Removed the led_blink_hdl() function (declaration,
> > > > > > > > > definition,
> > > > > > > > > and
> > > > > > > > > caller code) because it's useless. It only seems to check
> > > > > > > > > whether
> > > > > > > > > or
> > > > > > > > > not a given pointer is NULL. There are other (simpler)
> > > > > > > > > means
> > > > > > > > > for
> > > > > > > > > that
> > > > > > > > > purpose.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Fabio M. De Francesco
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 1 -
> > > > > > > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9
> > > > > > > > >  -
> > > > > > > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > > > > > >  3 files changed, 11 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > > > > > 0297fbad7bce..4c44dfd21514 100644
> > > > > > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > > > > > > > >
> > > > > > > > >   GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > > > > > > >   GEN_MLME_EXT_HANDLER(sizeof(struct
> SetChannelPlan_param),
> > > > > > > > >   set_chplan_hdl) /*59*/>
> > > > > > > > >
> > > > > > > > > - GEN_MLME_EXT_HANDLER(sizeof(struct
> LedBlink_param),
> > > > > > >
> > > > > > > led_blink_hdl)
> > > > > > >
> > > > > > > > > /*60*/
> > > > > > > >
> > > > > > > > This is worrisome.  Doyou fully understand the impact of
> > > > > > > > this?
> > > > > > > > If
> > > > > > > > not,
> > > > > > > > the change is probably not a good idea.
> > > > > > >
> > > > > > > This is that macro definition:
> > > > > > >
> > > > > > > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > > > > > >
> > > > > > > struct C2HEvent_Header {
> > > > > > >
> > > > > > > #ifdef __LITTLE_ENDIAN
> > > > > > >
> > > > > > > unsigned int len:16;
> > > > > > > unsigned int ID:8;
> > > > > > > unsigned int seq:8;
> > > > > > >
> > > > > > > #else
> > > > > > >
> > > > > > > unsigned int seq:8;
> > > > > > > unsigned int ID:8;
> > > > > > > unsigned int len:16;
> > > > > > >
> > > > > > > #endif
> > > > > > >
> > > > > > > unsigned int rsvd;
> > > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > It

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> On Tuesday, April 13, 2021 8:20:50 PM CEST Dan Carpenter wrote:
> > On Tue, Apr 13, 2021 at 06:47:06PM +0200, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > Removed the led_blink_hdl() function (declaration, definition,
> > > > > > > and
> > > > > > > caller code) because it's useless. It only seems to check
> > > > > > > whether
> > > > > > > or
> > > > > > > not a given pointer is NULL. There are other (simpler) means
> > > > > > > for
> > > > > > > that
> > > > > > > purpose.
> > > > > > >
> > > > > > > Signed-off-by: Fabio M. De Francesco 
> > > > > > > ---
> > > > > > >
> > > > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 1 -
> > > > > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> > > > > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > > > >  3 files changed, 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > > > 0297fbad7bce..4c44dfd21514 100644
> > > > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > > > > > >
> > > > > > >   GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > > > > >   GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> > > > > > >   set_chplan_hdl) /*59*/>
> > > > > > >
> > > > > > > - GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> > > > >
> > > > > led_blink_hdl)
> > > > >
> > > > > > > /*60*/
> > > > > >
> > > > > > This is worrisome.  Doyou fully understand the impact of this?
> > > > > > If
> > > > > > not,
> > > > > > the change is probably not a good idea.
> > > > >
> > > > > This is that macro definition:
> > > > >
> > > > > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > > > >
> > > > > struct C2HEvent_Header {
> > > > >
> > > > > #ifdef __LITTLE_ENDIAN
> > > > >
> > > > > unsigned int len:16;
> > > > > unsigned int ID:8;
> > > > > unsigned int seq:8;
> > > > >
> > > > > #else
> > > > >
> > > > > unsigned int seq:8;
> > > > > unsigned int ID:8;
> > > > > unsigned int len:16;
> > > > >
> > > > > #endif
> > > > >
> > > > > unsigned int rsvd;
> > > > >
> > > > > };
> > > > >
> > > > > It's a bit convoluted with regard to my experience. Probably I
> > > > > don't
> > > > > understand it fully, but it seems to me to not having effects to
> > > > > the
> > > > > code where I removed its use within core/rtw_cmd.c.
> > > > >
> > > > > What am I missing?
> > > >
> > > > It seems that the function is being put into an array.  Probably
> > > > someone
> > > > expects to find it there.  Probably you have shifted all of the
> > > > functions that come afterwards back one slot so that they are all in
> > > > the wrong places.
> > > >
> > > > julia
> > >
> > > Thanks for your explanation. Obviously this implies that the function
> > > cannot be removed, unless one fill the slot that is deleted by to not
> > > calling this macro at the right moment.
> > >
> > > I also suppose that providing a function pointer with a NULL value
> > > wouldn't work either.
> >
> > It would work.  That array is full of NULL function pointers.
> >
> Interesting, thanks.
>
> I'm going to remove that function and replace its name in the macro with a
> NULL function pointer.
>
> I couldn't believe it would work when I wrote about that.

Have you checked that a value of NULL in that place is going to have the
same effect as the function?

julia

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > Removed the led_blink_hdl() function (declaration, definition, and
> > > caller code) because it's useless. It only seems to check whether or
> > > not a given pointer is NULL. There are other (simpler) means for that
> > > purpose.
> > >
> > > Signed-off-by: Fabio M. De Francesco 
> > > ---
> > >
> > >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 1 -
> > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > >  3 files changed, 11 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > 0297fbad7bce..4c44dfd21514 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > >
> > >   GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > >   GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> > >   set_chplan_hdl) /*59*/>
> > > - GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> led_blink_hdl)
> > > /*60*/
> > This is worrisome.  Doyou fully understand the impact of this?  If not,
> > the change is probably not a good idea.
> >
> This is that macro definition:
>
> #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
>
> struct C2HEvent_Header {
>
> #ifdef __LITTLE_ENDIAN
>
> unsigned int len:16;
> unsigned int ID:8;
> unsigned int seq:8;
> #else
> unsigned int seq:8;
> unsigned int ID:8;
> unsigned int len:16;
> #endif
> unsigned int rsvd;
> };
>
> It's a bit convoluted with regard to my experience. Probably I don't
> understand it fully, but it seems to me to not having effects to the code
> where I removed its use within core/rtw_cmd.c.
>
> What am I missing?

It seems that the function is being put into an array.  Probably someone
expects to find it there.  Probably you have shifted all of the functions
that come afterwards back one slot so that they are all in the wrong
places.

julia

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> Removed the led_blink_hdl() function (declaration, definition, and
> caller code) because it's useless. It only seems to check whether or not a
> given pointer is NULL. There are other (simpler) means for that purpose.
>
> Signed-off-by: Fabio M. De Francesco 
> ---
>  drivers/staging/rtl8723bs/core/rtw_cmd.c | 1 -
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
>  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
>  3 files changed, 11 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..4c44dfd21514 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
>
>   GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
>   GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param), 
> set_chplan_hdl) /*59*/
> - GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), led_blink_hdl) 
> /*60*/

This is worrisome.  Doyou fully understand the impact of this?  If not,
the change is probably not a good idea.

julia

>
>   GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelSwitch_param), 
> set_csa_hdl) /*61*/
>   GEN_MLME_EXT_HANDLER(sizeof(struct TDLSoption_param), tdls_hdl) /*62*/
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 440e22922106..6f2a0584f977 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -6189,15 +6189,6 @@ u8 set_chplan_hdl(struct adapter *padapter, unsigned 
> char *pbuf)
>   return  H2C_SUCCESS;
>  }
>
> -u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf)
> -{
> -
> - if (!pbuf)
> - return H2C_PARAMETERS_ERROR;
> -
> - return  H2C_SUCCESS;
> -}
> -
>  u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf)
>  {
>   return  H2C_REJECTED;
> diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h 
> b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> index 5e6cf63956b8..472818c5fd83 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> @@ -745,7 +745,6 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned 
> char *pbuf);
>  u8 tx_beacon_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 set_ch_hdl(struct adapter *padapter, u8 *pbuf);
>  u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf);
> -u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf);   /* 
> Kurt: Handling DFS channel switch announcement ie. */
>  u8 tdls_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 run_in_thread_hdl(struct adapter *padapter, u8 *pbuf);
> --
> 2.31.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210413155908.8691-1-fmdefrancesco%40gmail.com.
>

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: core: Remove unused but set variable

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> Removed "ledBlink_param" because it was set to the value of "pbuf" but was
> never reused. This set was made by direct assignment (no helper had been
> called), therefore it had no side effect to the location pointed by "pbuf".
>
> Signed-off-by: Fabio M. De Francesco 
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index f19a15a3924b..440e22922106 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -6191,12 +6191,10 @@ u8 set_chplan_hdl(struct adapter *padapter, unsigned 
> char *pbuf)
>
>  u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf)
>  {
> - struct LedBlink_param *ledBlink_param;
>
>   if (!pbuf)
>   return H2C_PARAMETERS_ERROR;
>
> - ledBlink_param = (struct LedBlink_param *)pbuf;
>   return  H2C_SUCCESS;
>  }

Is this function actually useful?

julia

Re: cocci script hints request

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio Aiuto wrote:

> Hi,
>
> I would like to improve the following coccinelle script:
>
> @@
> expression a, fmt;
> expression list var_args;
> @@
>
> -   DBG_871X_LEVEL(a, fmt, var_args);
> +   printk(fmt, var_args);
>
> I would  replace the DBG_871X_LEVEL macro with printk, but
> I can't find a way to add KERN_* constant prefix to the fmt
> argument in the + code line. If i try this
>
> @@
> expression a, fmt;
> expression list var_args;
> @@
>
> -   DBG_871X_LEVEL(a, fmt, var_args);
> +   printk(KERN_DEBUG fmt, var_args);
>
> plus: parse error:
>   File "../test.cocci", line 94, column 20, charpos = 1171
>   around = 'fmt',
>   whole content = +   printk(KERN_DEBUG fmt, var_args);
>
> how could I do this?

Although I certainly agree with Greg, I'll answer the question from a
technical point of view.

I'm not sure that that kind of compound string is supported for a
metavariable.  It is possible to get around this problem using a python
script.  If you ever need to do this for a better reason, you can take a
look at demos/pythontococci.cocci in the Coccinelle source code
distribution.

julia

Re: [Outreachy kernel] Subject: [PATCH v2] staging: media: meson: vdec: declare u32 as static const appropriately

[Julia Lawall]

On Tue, 13 Apr 2021, Mitali Borkar wrote:

> Declared 32 bit unsigned int as static constant inside a function
> appropriately.

I don't think that the description matches what is done.  Perhaps all the
meaning is intended to be in the word "appropriately", but that is not
very clear.  The message makes it looks like static const is the new part,
but it is already there.

julia

>
> Reported-by: kernel test robot 
> Signed-off-by: Mitali Borkar 
> ---
>
> Changes from v1:- Rectified the mistake by declaring u32 as static const
> properly.
>
>  drivers/staging/media/meson/vdec/codec_h264.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/meson/vdec/codec_h264.c 
> b/drivers/staging/media/meson/vdec/codec_h264.c
> index ea86e9e1c447..80141b89a9f6 100644
> --- a/drivers/staging/media/meson/vdec/codec_h264.c
> +++ b/drivers/staging/media/meson/vdec/codec_h264.c
> @@ -287,8 +287,8 @@ static void codec_h264_resume(struct amvdec_session *sess)
>   struct amvdec_core *core = sess->core;
>   struct codec_h264 *h264 = sess->priv;
>   u32 mb_width, mb_height, mb_total;
> - static const u32[] canvas3 = { ANCO_CANVAS_ADDR, 0 };
> - static const u32[] canvas4 = { 24, 0 };
> + static const u32 canvas3[] = { ANCO_CANVAS_ADDR, 0 };
> + static const u32 canvas4[] = { 24, 0 };
>
>   amvdec_set_canvases(sess, canvas3, canvas4);
>
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHU56OM%2BC2zY34VP%40kali.
>

Re: [Outreachy kernel][PATCH 1/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss.c

[Julia Lawall]

On Mon, 12 Apr 2021, ascordeiro wrote:

> Em seg, 2021-04-12 às 18:11 +0300, Laurent Pinchart escreveu:
> > Hi Aline,
> >
> > On Mon, Apr 12, 2021 at 10:58:45AM -0300, ascordeiro wrote:
> > > Em seg, 2021-04-12 às 16:40 +0300, Laurent Pinchart escreveu:
> > > > While testing on a device isn't a requirement as you can't be
> > > > expected
> > > > to have the necessary hardware, changes are expected to be at
> > > > least
> > > > compile-tested before being submitted.
> > >
> > > Hi Laurent,
> > >
> > > I thought recompiling the kernel would be enough in this case.
> > > I recompiled it in native Ubuntu 16.04 without errors.
> >
> > Did it compile the driver you modified ?
> >
> I'm sorry, It didn't. I forgot to enable the option to compile the
> driver as a module in "make menuconfig" and now I'm seeing the problems
> I generated.

You can easily compile a single file using make path/foo.o and a single
directory using make path/.

julia

Re: [Outreachy kernel] Re: [PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Julia Lawall]

On Mon, 12 Apr 2021, Greg KH wrote:

> On Mon, Apr 12, 2021 at 12:42:30PM +0300, Sakari Ailus wrote:
> > Hi Mitali,
> >
> > On Mon, Apr 12, 2021 at 04:38:39AM +0530, Mitali Borkar wrote:
> > > Added #include  and replaced bit shifts by BIT() macro.
> > > This BIT() macro from linux/bitops.h is used to define 
> > > IPU3_UAPI_GRID_Y_START_EN
> > > and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
> > > Use of macro is better and neater. It maintains consistency.
> > > Reported by checkpatch.
> > >
> > > Signed-off-by: Mitali Borkar 
> > > ---
> > >  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h 
> > > b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > index edd8edda0647..589d5ccee3a7 100644
> > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > @@ -5,6 +5,7 @@
> > >  #define __IPU3_UAPI_H
> > >
> > >  #include 
> > > +#include 
> > >
> > >  /* from /drivers/staging/media/ipu3/include/videodev2.h */
> > >
> > > @@ -22,11 +23,11 @@
> > >  #define IPU3_UAPI_MAX_BUBBLE_SIZE10
> > >
> > >  #define IPU3_UAPI_GRID_START_MASK((1 << 12) - 1)

Mitali,

It's not very relevant given the use of this header, but if the 1 << 15
translation was correct, you could have changed the 1 << 12 computation as
well.

julia

> > > -#define IPU3_UAPI_GRID_Y_START_EN(1 << 15)
> > > +#define IPU3_UAPI_GRID_Y_START_ENBIT(15)
> >
> > This header is used in user space where you don't have the BIT() macro.
>
> If that is true, why is it not in a "uapi" subdir within this driver?
>
> Otherwise it is not obvious at all that this is the case :(
>
> thanks,
>
> greg k-h
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHQXty07oAP1L0W9%40kroah.com.
>

Re: [Outreachy kernel] [PATCH] Staging: Remove line to fix checkpatch error

[Julia Lawall]

On Sun, 11 Apr 2021, tawahpeggy wrote:

> remove one empty line.CHECK: Please don't use multiple blank lines

Did something go wrong with the patch generation?  You say that you are
removing one line, but the diff information looks like you are adding a
file.  Normally a patch has only the changed lines and a few lines before
and after.

> Signed-off-by: tawahpeggy 

You need to put your real name when contributing to the Linux kernel.

For example, I would put:

Julia Lawall 

julia

>
> ---
>  drivers/staging/comedi/comedi_pcmcia.mod.c | 1 -
>  1 file changed, 0 insertion(+), 1 deletion(-)
>  create mode 100644 drivers/staging/comedi/comedi_pcmcia.mod.c
>
> diff --git a/drivers/staging/comedi/comedi_pcmcia.mod.c 
> b/drivers/staging/comedi/comedi_pcmcia.mod.c
> index 0904b8765afs96..3984db1a39c8
> --- /dev/null
> +++ b/drivers/staging/comedi/comedi_pcmcia.mod.c
> @@ -0,0 +1,31 @@
> #include 
> #define INCLUDE_VERMAGIC
> #include 
> #include 
> #include 
>
> BUILD_SALT;
>
> MODULE_INFO(vermagic, VERMAGIC_STRING);
> MODULE_INFO(name, KBUILD_MODNAME);
>
> __visible struct module __this_module
> __section(".gnu.linkonce.this_module") = {
>   .name = KBUILD_MODNAME,
>   .init = init_module,
> #ifdef CONFIG_MODULE_UNLOAD
>   .exit = cleanup_module,
> #endif
>   .arch = MODULE_ARCH_INIT,
> };
>
> #ifdef CONFIG_RETPOLINE
> MODULE_INFO(retpoline, "Y");
> #endif
>
> MODULE_INFO(staging, "Y");
>
> MODULE_INFO(depends, "pcmcia,comedi");
>
> -
> MODULE_INFO(srcversion, "ED971F2E01020DFA2B04486");
> --
> 2.17.1
> h-Hartman 
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210411204933.GA3524%40peggy-Lenovo-V130-15IKB.
>

Re: [Outreachy kernel] [PATCH] staging: rtl8192u: Remove variable set but not used

[Julia Lawall]

On Sun, 11 Apr 2021, Fabio M. De Francesco wrote:

> Remove variable "int ret" which is instantiated but not used.

instantiated -> declared?  I thought instantiated could mean initialized,
but that doesn't seem to be the case.

julia

>
> Signed-off-by: Fabio M. De Francesco 
> ---
>  drivers/staging/rtl8192u/r8192U_core.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c 
> b/drivers/staging/rtl8192u/r8192U_core.c
> index f48186a89fa1..30055de66239 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -902,7 +902,6 @@ static void rtl8192_hard_data_xmit(struct sk_buff *skb, 
> struct net_device *dev,
>  int rate)
>  {
>   struct r8192_priv *priv = (struct r8192_priv *)ieee80211_priv(dev);
> - int ret;
>   unsigned long flags;
>   struct cb_desc *tcb_desc = (struct cb_desc *)(skb->cb + 
> MAX_DEV_ADDR_SIZE);
>   u8 queue_index = tcb_desc->queue_index;
> --
> 2.31.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210411174143.31618-1-fmdefrancesco%40gmail.com.
>

Re: [Outreachy kernel] [PATCH v3 4/4] staging: rtl8723bs: core: Change a controlling expression

[Julia Lawall]

On Sun, 11 Apr 2021, Fabio M. De Francesco wrote:

> On Sunday, April 11, 2021 11:51:32 AM CEST Julia Lawall wrote:
> > On Sun, 11 Apr 2021, Fabio M. De Francesco wrote:
> > > On Sunday, April 11, 2021 11:26:41 AM CEST Julia Lawall wrote:
> > > > On Sun, 11 Apr 2021, Fabio M. De Francesco wrote:
> > > > > Change a controlling expression within an 'if' statement: don't
> > > > > compare
> > > > > with 'true'.
> > > > >
> > > > > Signed-off-by: Fabio M. De Francesco 
> > > > > ---
> > > > >
> > > > > Changes from v2: Rewrite subject in patch 0/4; remove a patch from
> > > > > the
> > > > > series because it had already been applied (rtl8723bs: core: Remove
> > > > > an
> > > > > unused variable). Changes from v1: Fix a typo in subject of patch
> > > > > 1/5,
> > > > > add patch 5/5.>
> > > > >
> > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > 32079e0f71d5..600366cb1aeb 100644
> > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > @@ -1507,7 +1507,7 @@ static void rtw_lps_change_dtim_hdl(struct
> > > > > adapter *padapter, u8 dtim)>
> > > > >
> > > > >   if (pwrpriv->dtim != dtim)
> > > > >
> > > > >   pwrpriv->dtim = dtim;
> > > > >
> > > > > - if ((pwrpriv->fw_current_in_ps_mode == true) && (pwrpriv-
> > > >
> > > >pwr_mode >
> > > >
> > > > > PS_MODE_ACTIVE)) { +  if ((pwrpriv->fw_current_in_ps_mode) &&
> > > > > (pwrpriv->pwr_mode > PS_MODE_ACTIVE)) {
> > > >
> > > > The parentheses in the left argument of && can be dropped as well.
> > >
> > > What about the parentheses of the right argument? I'm not sure: does
> > > '>'
> > > have precedence over '&&'? Doesn't it?
> >
> > On the right they are not actually needed either:
> >
> So, I remembered well :)
> >
> > https://en.cppreference.com/w/c/language/operator_precedence
> >
> Very nice table. Thanks for the link.
> >
> > But you could look around in the code and see what people typically do.
> > Perhaps one might find the parentheses more clear when there is a binary
> > operator.  But when there is no binary operator, they could be more
> > confusing than useful.
> >
> When I look around in the code I see a lot of unnecessary parentheses.
> What people typically do is not always the right thing. I prefer to remove
> parentheses where they are redundant.

Not sure I was clear.  This driver seems to be very enthusiastic about
parenttheses.  But perhaps check in other more mature parts of the kernel.

julia

>
> Thanks for your kind help,
>
> Fabio
> >
> > julia
> >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > julia
> > > >
> > > > >   u8 ps_mode = pwrpriv->pwr_mode;
> > > > >
> > > > >   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE,
> > >
> > > (u8
> > >
> > > > >   *)(_mode));
> > > > >
> > > > > --
> > > > > 2.31.1
> > > > >
> > > > > --
> > > > > 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+unsubscr...@googlegroups.com. To view this
> > > > > discussion
> > > > > on the web visit
> > > > > https://groups.google.com/d/msgid/outreachy-kernel/20210411082908.3
> > > > > 187
> > > > > 6-5-fmdefrancesco%40gmail.com.
>
>
>
>
>

Re: [Outreachy kernel] [PATCH v3 1/3] staging: rtl8192e: remove parentheses around boolean expression

[Julia Lawall]

On Sun, 11 Apr 2021, Mitali Borkar wrote:

> Removed unnecessary parentheses around '!xyz' boolean expression as '!' has 
> higher
> precedance than '||'

The log message is too wide.  It should be at most around 70 characters
wide, because git will indent it a little.

julia


>
> Signed-off-by: Mitali Borkar 
> ---
>
> Changes from v2:- Modified subject description.
> Changes from v1:- Removed unnecessary parentheses around boolean expression.
> Changes has been made in v2.
>
>  drivers/staging/rtl8192e/rtl819x_HTProc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c 
> b/drivers/staging/rtl8192e/rtl819x_HTProc.c
> index b1fa8e9a4f28..431202927036 100644
> --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c
> @@ -276,7 +276,7 @@ void HTConstructCapabilityElement(struct rtllib_device 
> *ieee, u8 *posHTCap,
>   struct rt_hi_throughput *pHT = ieee->pHTInfo;
>   struct ht_capab_ele *pCapELE = NULL;
>
> - if ((!posHTCap) || (!pHT)) {
> + if (!posHTCap || !pHT) {
>   netdev_warn(ieee->dev,
>   "%s(): posHTCap and pHTInfo are null\n", __func__);
>   return;
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/30a330377667aa5043a60ed3cdf1bbb37099631c.1618133351.git.mitaliborkar810%40gmail.com.
>

Re: [Outreachy kernel] [PATCH v3 4/4] staging: rtl8723bs: core: Change a controlling expression

[Julia Lawall]

On Sun, 11 Apr 2021, Fabio M. De Francesco wrote:

> On Sunday, April 11, 2021 11:26:41 AM CEST Julia Lawall wrote:
> > On Sun, 11 Apr 2021, Fabio M. De Francesco wrote:
> > > Change a controlling expression within an 'if' statement: don't compare
> > > with 'true'.
> > >
> > > Signed-off-by: Fabio M. De Francesco 
> > > ---
> > >
> > > Changes from v2: Rewrite subject in patch 0/4; remove a patch from the
> > > series because it had already been applied (rtl8723bs: core: Remove an
> > > unused variable). Changes from v1: Fix a typo in subject of patch 1/5,
> > > add patch 5/5.>
> > >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > 32079e0f71d5..600366cb1aeb 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > @@ -1507,7 +1507,7 @@ static void rtw_lps_change_dtim_hdl(struct
> > > adapter *padapter, u8 dtim)>
> > >   if (pwrpriv->dtim != dtim)
> > >
> > >   pwrpriv->dtim = dtim;
> > >
> > > - if ((pwrpriv->fw_current_in_ps_mode == true) && (pwrpriv-
> >pwr_mode >
> > > PS_MODE_ACTIVE)) { +  if ((pwrpriv->fw_current_in_ps_mode) &&
> > > (pwrpriv->pwr_mode > PS_MODE_ACTIVE)) {
> > The parentheses in the left argument of && can be dropped as well.
> >
> What about the parentheses of the right argument? I'm not sure: does '>'
> have precedence over '&&'? Doesn't it?

On the right they are not actually needed either:

https://en.cppreference.com/w/c/language/operator_precedence

But you could look around in the code and see what people typically do.
Perhaps one might find the parentheses more clear when there is a binary
operator.  But when there is no binary operator, they could be more
confusing than useful.

julia

>
> Thanks,
>
> Fabio
> >
> > julia
> >
> > >   u8 ps_mode = pwrpriv->pwr_mode;
> > >
> > >   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE,
> (u8
> > >   *)(_mode));
> > >
> > > --
> > > 2.31.1
> > >
> > > --
> > > 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+unsubscr...@googlegroups.com. To view this discussion
> > > on the web visit
> > > https://groups.google.com/d/msgid/outreachy-kernel/20210411082908.3187
> > > 6-5-fmdefrancesco%40gmail.com.
>
>
>
>
>

Re: [Outreachy kernel] [PATCH v3 4/4] staging: rtl8723bs: core: Change a controlling expression

[Julia Lawall]

On Sun, 11 Apr 2021, Fabio M. De Francesco wrote:

> Change a controlling expression within an 'if' statement: don't compare
> with 'true'.
>
> Signed-off-by: Fabio M. De Francesco 
> ---
>
> Changes from v2: Rewrite subject in patch 0/4; remove a patch from the
> series because it had alreay been applied (rtl8723bs: core: Remove an unused 
> variable).
> Changes from v1: Fix a typo in subject of patch 1/5, add patch 5/5.
>
>  drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 32079e0f71d5..600366cb1aeb 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -1507,7 +1507,7 @@ static void rtw_lps_change_dtim_hdl(struct adapter 
> *padapter, u8 dtim)
>   if (pwrpriv->dtim != dtim)
>   pwrpriv->dtim = dtim;
>
> - if ((pwrpriv->fw_current_in_ps_mode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
> + if ((pwrpriv->fw_current_in_ps_mode) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {

The parentheses in the left argument of && can be dropped as well.

julia

>   u8 ps_mode = pwrpriv->pwr_mode;
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> --
> 2.31.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210411082908.31876-5-fmdefrancesco%40gmail.com.
>

Re: [Outreachy kernel] [PATCH v3] staging: rtl8192e: fixed pointer error by adding '*'

[Julia Lawall]

On Sat, 10 Apr 2021, Mitali Borkar wrote:

> Fixed pointer error by adding '*' to the function.
> Reported by Julia.

Actually, there is a proper tag for reported by, like Signed-off-by.  Look
through the git history to see what to do.

julia

>
> Signed-off-by: Mitali Borkar 
> ---
> Changes from v2:- modified patch body but writing commit message
> clearly.
> Changes from v1:- added pointer to the function.
>
>  drivers/staging/rtl8192e/rtl819x_TSProc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c 
> b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> index 4457c1acfbf6..78b5b4eaec5f 100644
> --- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> @@ -327,7 +327,7 @@ bool GetTs(struct rtllib_device *ieee, struct 
> ts_common_info **ppTS,
>   }
>
>   *ppTS = SearchAdmitTRStream(ieee, Addr, UP, TxRxSelect);
> - if (ppTS)
> + if (*ppTS)
>   return true;
>
>   if (!bAddNewTs) {
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHGvJxMhQ8nzHf6I%40kali.
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 3:24:43 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > > > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > > > That variable has global scope and is assigned at least in:
> > > > > > What do you mean by global scope?  None of the following look
> > > > > > like
> > > > > > references to global variables.
> > > > > >
> > > > > > julia
> > > > >
> > > > > I just mean that fw_current_in_ps_mode is a field of a struct in a
> > > > > .h
> > > > > file included everywhere in this driver and that the functions whom
> > > > > the following assignments belong to have not the "static" type
> > > > > modifier.
> > > >
> > > > OK, but a field in a structure is not a variable, and this is not
> > > > what
> > > > scope means.
> > >
> > > You're right, a field in a structure is not a variable.
> > >
> > > > int x;
> > > >
> > > > outside of anything is a global variable (global scope).
> > > >
> > > > int foo() {
> > > >
> > > >   int x;
> > > >   ...
> > > >
> > > > }
> > > >
> > > > Here x is a local variable.  Its scope is the body of the function.
> > > >
> > > > int foo() {
> > > >
> > > >   if (abc) {
> > > >
> > > > int x;
> > > > ...
> > > >
> > > >   }
> > > >
> > > > }
> > > >
> > > > Here x is a local variable, but its scope is only in the if branch.
> > >
> > > And you're right again: I needed a little refresh of my knowledge of C.
> > >
> > > I've searched again in the code for the purpose of finding out if that
> > > struct is initialized with global scope but I didn't find anything. I
> > > didn't even find any dynamic allocation within functions that returns
> > > pointers to that struct.
> > >
> > > Therefore, according to Greg's request, I'll delete that stupid 'if'
> > > statement in the patch series v2 that I'm about to submit.
> >
> > I'm really not clear on why the if should be deleted.
> >
> > julia
> >
> I'm supposed to delete it because of the review made by Greg. In a couple
> of previous messages he wrote:
>
> "If this is only checked, how can it ever be true?  Who ever sets this
> value?"
>
> and then:
>
> "Just delete the variable from the structure entirely and when it is
> used.".
>
> However, like you, I'm not sure yet.

Be sure.  Greg already said that he misinterpreted the patch, because he
thought that the name also changed.

julia


>
> Thanks,
>
> Fabio
> >
> > > I've really appreciated your help.
> > >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > julia
> > > >
> > > > > Thanks,
> > > > >
> > > > > Fabio
> > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH v2 2/2] staging: media: zoran: remove and add comments; align code

[Julia Lawall]

> Mam, my new patch is ready but I am not sure how to send, the v1/2 is
> not on top, I have not made changes in it because it was not required. Now,  
> how to
> send these two mails as a patchset, since in between these two git
> commits, I have another commits too.

Suppose you have

relevant patch1
patch on some other file
relevant patch2

Then you can use git rebase -i HEAD~3 to reorder them, ie to put relevant
patch2 before patch on some other file.  When you do the git rebase, just
move the lines for the different patches around and save the changes.

julia

>
> > julia
> >
> > > > > +#define ZR36057_VFESPFR_RGB565   (2 << 3)
> > > > > +#define ZR36057_VFESPFR_RGB555   (3 << 3)
> > > > > +#define ZR36057_VFESPFR_ERR_DIF  BIT(2)
> > > > > +#define ZR36057_VFESPFR_PACK24   BIT(1)
> > > > > +#define ZR36057_VFESPFR_LITTLE_ENDIANBIT(0)
> > > > > +
> > > > > +/* Video Display "Top" Register */
> > > > > +#define ZR36057_VDTR 0x00c
> > > > > +
> > > > > +/* Video Display "Bottom" Register */
> > > > > +#define ZR36057_VDBR 0x010
> > > > > +
> > > > > +/* Video Stride, Status, and Frame Grab Register */
> > > > > +#define ZR36057_VSSFGR   0x014
> > > > > +#define ZR36057_VSSFGR_DISP_STRIDE   16
> > > > > +#define ZR36057_VSSFGR_VID_OVF   BIT(8)
> > > > > +#define ZR36057_VSSFGR_SNAP_SHOT BIT(1)
> > > > > +#define ZR36057_VSSFGR_FRAME_GRABBIT(0)
> > > > > +
> > > > > +/* Video Display Configuration Register */
> > > > > +#define ZR36057_VDCR 0x018
> > > > > +#define ZR36057_VDCR_VID_EN  BIT(31)
> > > > > +#define ZR36057_VDCR_MIN_PIX 24
> > > > > +#define ZR36057_VDCR_TRITON  BIT(24)
> > > > > +#define ZR36057_VDCR_VID_WIN_HT  12
> > > >
> > > > These don't look well aligned either.
> > > >
> > > > Please check on the rest.
> > > >
> > > Yes Ma'am, I am rechecking this.
> > >
> > > > julia
> > > >
> > > >
> > > > > +#define ZR36057_VDCR_VID_WIN_WID 0
> > > > > +
> > > > > +/* Masking Map "Top" Register */
> > > > > +#define ZR36057_MMTR 0x01c
> > > > > +
> > > > > +/* Masking Map "Bottom" Register */
> > > > > +#define ZR36057_MMBR 0x020
> > > > > +
> > > > > +/* Overlay Control Register */
> > > > > +#define ZR36057_OCR  0x024
> > > > > +#define ZR36057_OCR_OVL_ENABLE   BIT(15)
> > > > > +#define ZR36057_OCR_MASK_STRIDE  0
> > > > > +
> > > > > +/* System, PCI, and General Purpose Pins Control Register */
> > > > > +#define ZR36057_SPGPPCR  0x028
> > > > > +#define ZR36057_SPGPPCR_SOFT_RESET   BIT(24)
> > > > > +
> > > > > +/* General Purpose Pins and GuestBus Control Register (1) */
> > > > > +#define ZR36057_GPPGCR1  0x02c
> > > > > +
> > > > > +/* MPEG Code Source Address Register */
> > > > > +#define ZR36057_MCSAR0x030
> > > > > +
> > > > > +/* MPEG Code Transfer Control Register */
> > > > > +#define ZR36057_MCTCR0x034
> > > > > +#define ZR36057_MCTCR_COD_TIME   BIT(30)
> > > > > +#define ZR36057_MCTCR_C_EMPTYBIT(29)
> > > > > +#define ZR36057_MCTCR_C_FLUSHBIT(28)
> > > > >  #define ZR36057_MCTCR_COD_GUEST_ID   20
> > > > >  #define ZR36057_MCTCR_COD_GUEST_REG  16
> > > > >
> > > > > -#define ZR36057_MCMPR   0x038/* MPEG Code Memory 
> > > > > Pointer Register */
> > > > > -
> > > > > -#define ZR36057_ISR 0x03c/* Interrupt Status 
> > > > > Register */
> > > > > -#define ZR36057_ISR_GIRQ1BIT(30)
> > > > > -#define ZR36057_ISR_GIRQ0BIT(29)
> > > > > -#define ZR36057_ISR_COD_REP_IRQBIT(28)
> > > > > -#define ZR36057_ISR_JPEG_REP_IRQ   BIT(27)
> > > > > -
> > > > > -#define ZR36057_ICR 0x040/* Interrupt Control 
> > > > > Register */
> > > > > -#define ZR36057_ICR_GIRQ1BIT(30)
> > > > > -#define ZR36057_ICR_GIRQ0BIT(29)
> > > > > -#define ZR36057_ICR_COD_REP_IRQBIT(28)
> > > > > -#define ZR36057_ICR_JPEG_REP_IRQ   BIT(27)
> > > > > -#define ZR36057_ICR_INT_PIN_EN BIT(24)
> > > > > -
> > > > > -#define ZR36057_I2CBR   0x044/* I2C Bus Register */
> > > > > -#define ZR36057_I2CBR_SDA BIT(1)
> > > > > -#define ZR36057_I2CBR_SCL BIT(0)
> > > > > -
> > > > > -#define ZR36057_JMC 0x100/* JPEG Mode and 
> > > > > Control */
> > > > > -#define ZR36057_JMC_JPG  BIT(31)
> > > > > -#define ZR36057_JMC_JPG_EXP_MODE  (0 << 29)
> > > > > -#define ZR36057_JMC_JPG_CMP_MODE   BIT(29)
> > > > > -#define ZR36057_JMC_MJPG_EXP_MODE (2 << 29)
> > > > > 

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > That variable has global scope and is assigned at least in:
> > > > What do you mean by global scope?  None of the following look like
> > > > references to global variables.
> > > >
> > > > julia
> > >
> > > I just mean that fw_current_in_ps_mode is a field of a struct in a .h
> > > file included everywhere in this driver and that the functions whom
> > > the following assignments belong to have not the "static" type
> > > modifier.
> > OK, but a field in a structure is not a variable, and this is not what
> > scope means.
> >
> You're right, a field in a structure is not a variable.
> >
> > int x;
> >
> > outside of anything is a global variable (global scope).
> >
> > int foo() {
> >   int x;
> >   ...
> > }
> >
> > Here x is a local variable.  Its scope is the body of the function.
> >
> > int foo() {
> >   if (abc) {
> > int x;
> > ...
> >   }
> > }
> >
> > Here x is a local variable, but its scope is only in the if branch.
> >
> And you're right again: I needed a little refresh of my knowledge of C.
>
> I've searched again in the code for the purpose of finding out if that
> struct is initialized with global scope but I didn't find anything. I
> didn't even find any dynamic allocation within functions that returns
> pointers to that struct.
>
> Therefore, according to Greg's request, I'll delete that stupid 'if'
> statement in the patch series v2 that I'm about to submit.

I'm really not clear on why the if should be deleted.

julia


>
> I've really appreciated your help.
>
> Thanks,
>
> Fabio
> >
> > julia
> >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > >
> > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH v2] staging: rtl8192e: fixed pointer error by adding '*'

[Julia Lawall]

On Sat, 10 Apr 2021, Mitali Borkar wrote:

> Fixed Comparison to NULL can be written as '!...' by replacing it with
> simpler form i.e. boolean expression. This makes code more readable
> alternative.
> Reported by checkpatch.
>
> Signed-off-by: Mitali Borkar 
> ---
> Changes from v1:- added pointer to the function, which was missed during
> fixing v1.
>
>  drivers/staging/rtl8192e/rtl819x_TSProc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c 
> b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> index 4457c1acfbf6..78b5b4eaec5f 100644
> --- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> @@ -327,7 +327,7 @@ bool GetTs(struct rtllib_device *ieee, struct 
> ts_common_info **ppTS,
>   }
>
>   *ppTS = SearchAdmitTRStream(ieee, Addr, UP, TxRxSelect);
> - if (ppTS)
> + if (*ppTS)

This looks like a patch against your previous patch, and not against the
original code.

julia

>   return true;
>
>   if (!bAddNewTs) {
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHGhdtldqAlRsPHT%40kali.
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > That variable has global scope and is assigned at least in:
> > What do you mean by global scope?  None of the following look like
> > references to global variables.
> >
> > julia
> I just mean that fw_current_in_ps_mode is a field of a struct in a .h file
> included everywhere in this driver and that the functions whom the
> following assignments belong to have not the "static" type modifier.

OK, but a field in a structure is not a variable, and this is not what
scope means.

int x;

outside of anything is a global variable (global scope).

int foo() {
  int x;
  ...
}

Here x is a local variable.  Its scope is the body of the function.

int foo() {
  if (abc) {
int x;
...
  }
}

Here x is a local variable, but its scope is only in the if branch.

julia

>
> Thanks,
>
> Fabio
> >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > pwrpriv->fw_current_in_ps_mode = false;
> > >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > pwrpriv->fw_current_in_ps_mode = true;
> > >
> > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH] staging: rtl8192e: replace comparison to NULL by bool

[Julia Lawall]

On Sat, 10 Apr 2021, Mitali Borkar wrote:

> Fixed Comparison to NULL can be written as '!...' by replacing it with
> simpler form i.e boolean expression. This makes code more readable 
> alternative.
> Reported by checkpatch.
>
> Signed-off-by: Mitali Borkar 
> ---
>  drivers/staging/rtl8192e/rtl819x_TSProc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c 
> b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> index 65eac33aaa5b..476875125e87 100644
> --- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> @@ -269,12 +269,12 @@ static void MakeTSEntry(struct ts_common_info 
> *pTsCommonInfo, u8 *Addr,
>  {
>   u8  count;
>
> - if (pTsCommonInfo == NULL)
> + if (!pTsCommonInfo)
>   return;
>
>   memcpy(pTsCommonInfo->Addr, Addr, 6);
>
> - if (pTSPEC != NULL)
> + if (pTSPEC)
>   memcpy((u8 *)(&(pTsCommonInfo->TSpec)), (u8 *)pTSPEC,
>  sizeof(union tspec_body));
>
> @@ -328,7 +328,7 @@ bool GetTs(struct rtllib_device *ieee, struct 
> ts_common_info **ppTS,
>   }
>
>   *ppTS = SearchAdmitTRStream(ieee, Addr, UP, TxRxSelect);
> - if (*ppTS != NULL)
> + if (ppTS)

You lost a * here.

julia

>   return true;
>
>   if (!bAddNewTs) {
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHDnWpWztxeZospi%40kali.
>

Re: [Outreachy kernel] [PATCH v2 2/2] staging: media: zoran: remove and add comments; align code

[Julia Lawall]

On Sat, 10 Apr 2021, Mitali Borkar wrote:

> On Fri, Apr 09, 2021 at 10:12:12PM +0200, Julia Lawall wrote:
> >
> >
> > On Sat, 10 Apr 2021, Mitali Borkar wrote:
> >
> > > Removed comments from the same line and added them to new line above the
> > > blocks, aligned everything properly by using tabs to make code neater
> > > and improve readability.
> > >
> > > Signed-off-by: Mitali Borkar 
> > > ---
> > >  drivers/staging/media/zoran/zr36057.h | 293 ++
> > >  1 file changed, 162 insertions(+), 131 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/zoran/zr36057.h 
> > > b/drivers/staging/media/zoran/zr36057.h
> > > index 93075459f910..198d344a8879 100644
> > > --- a/drivers/staging/media/zoran/zr36057.h
> > > +++ b/drivers/staging/media/zoran/zr36057.h
> > > @@ -12,145 +12,176 @@
> > >
> > >  /* Zoran ZR36057 registers */
> > >
> > > -#define ZR36057_VFEHCR  0x000/* Video Front End, Horizontal 
> > > Configuration Register */
> > > -#define ZR36057_VFEHCR_HS_POL BIT(30)
> > > -#define ZR36057_VFEHCR_H_START   10
> > > +/* Video Front End, Horizontal Configuration Register */
> > > +#define ZR36057_VFEHCR   0x000
> > > +#define ZR36057_VFEHCR_HS_POLBIT(30)
> >
> > It looks like the alignment didn't work out here?  Check that the use of
> > tabs is the same as on the nearby lines.
> >
> Do I need to align BIT(30), 10, 0x000 and rest in same column or should I
> align them separately?

If there are a bunch of #defines with no blank line between them and
similar names (these all start with ZR36057_VFEHCR), then it can be nice
to line them all up.

>
> > > +#define ZR36057_VFEHCR_H_START   10
> > >  #define ZR36057_VFEHCR_H_END 0
> > >  #define ZR36057_VFEHCR_HMASK 0x3ff
> > >
> > > -#define ZR36057_VFEVCR  0x004/* Video Front End, Vertical 
> > > Configuration Register */
> > > -#define ZR36057_VFEVCR_VS_POL BIT(30)
> > > -#define ZR36057_VFEVCR_V_START   10
> > > +/* Video Front End, Vertical Configuration Register */
> > > +#define ZR36057_VFEVCR   0x004
> > > +#define ZR36057_VFEVCR_VS_POLBIT(30)
> > > +#define ZR36057_VFEVCR_V_START   10
> > >  #define ZR36057_VFEVCR_V_END 0
> > >  #define ZR36057_VFEVCR_VMASK 0x3ff
> > >
> > > -#define ZR36057_VFESPFR 0x008/* Video Front End, Scaler and 
> > > Pixel Format Register */
> > > -#define ZR36057_VFESPFR_EXT_FLBIT(26)
> > > -#define ZR36057_VFESPFR_TOP_FIELD BIT(25)
> > > -#define ZR36057_VFESPFR_VCLK_POL  BIT(24)
> > > -#define ZR36057_VFESPFR_H_FILTER 21
> > > -#define ZR36057_VFESPFR_HOR_DCM  14
> > > -#define ZR36057_VFESPFR_VER_DCM  8
> > > -#define ZR36057_VFESPFR_DISP_MODE6
> > > -#define ZR36057_VFESPFR_YUV422  (0 << 3)
> > > -#define ZR36057_VFESPFR_RGB888  BIT(3)
> > > -#define ZR36057_VFESPFR_RGB565  (2 << 3)
> > > -#define ZR36057_VFESPFR_RGB555  (3 << 3)
> > > -#define ZR36057_VFESPFR_ERR_DIF  BIT(2)
> > > -#define ZR36057_VFESPFR_PACK24  BIT(1)
> > > -#define ZR36057_VFESPFR_LITTLE_ENDIANBIT(0)
> > > -
> > > -#define ZR36057_VDTR0x00c/* Video Display "Top" Register 
> > > */
> > > -
> > > -#define ZR36057_VDBR0x010/* Video Display "Bottom" 
> > > Register */
> > > -
> > > -#define ZR36057_VSSFGR  0x014/* Video Stride, Status, and 
> > > Frame Grab Register */
> > > -#define ZR36057_VSSFGR_DISP_STRIDE   16
> > > -#define ZR36057_VSSFGR_VID_OVFBIT(8)
> > > -#define ZR36057_VSSFGR_SNAP_SHOT  BIT(1)
> > > -#define ZR36057_VSSFGR_FRAME_GRAB BIT(0)
> > > -
> > > -#define ZR36057_VDCR0x018/* Video Display Configuration 
> > > Register */
> > > -#define ZR36057_VDCR_VID_EN   BIT(31)
> > > -#define ZR36057_VDCR_MIN_PIX 24
> > > -#define ZR36057_VDCR_TRITON  BIT(24)
> > > -#define ZR36057_VDCR_VID_WIN_HT   12
> > > -#define ZR36057_VDCR_VID_WIN_WID  0
> > > -
> > > -#d

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

> That variable has global scope and is assigned at least in:

What do you mean by global scope?  None of the following look like
references to global variables.

julia

>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> pwrpriv->fw_current_in_ps_mode = false;
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> pwrpriv->fw_current_in_ps_mode = true;
>
> drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> pwrctrlpriv->fw_current_in_ps_mode = false;

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Julia Lawall wrote:

>
>
> On Sat, 10 Apr 2021, Greg KH wrote:
>
> > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > it is used everywhere as a bool and, accordingly, it should be
> > > declared as a bool. Shorten the controlling
> > > expression of an 'if' statement.
> > >
> > > Signed-off-by: Fabio M. De Francesco 
> > > ---
> > >  drivers/staging/rtl8723bs/hal/hal_intf.c| 2 +-
> > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c 
> > > b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > index 96fe172ced8d..8dc4dd8c6d4c 100644
> > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
> > >
> > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > >  {
> > > - if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
> > > + if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > >   if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > >   padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* 
> > > this function caller is in interrupt context */
> > >   }
> > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h 
> > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > index 0a48f1653be5..0767dbb84199 100644
> > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > >   u8 LpsIdleCount;
> > >   u8 power_mgnt;
> > >   u8 org_power_mgnt;
> > > - u8 fw_current_in_ps_mode;
> > > + bool fw_current_in_ps_mode;
> > >   unsigned long   DelayLPSLastTimeStamp;
> > >   s32 pnp_current_pwr_state;
> > >   u8 pnp_bstop_trx;
> >
> > If this is only checked, how can it ever be true?  Who ever sets this
> > value?
>
> I think it's already updated everywhere with true and false, so there is
> nothing to change.  But it would be good to make that clear in the log
> message.

Oops, I was thinking of the field, not the local variable.
If the field is never set, that seems like a big problem...

julia

>
> julia
>
> >
> > thanks,
> >
> > greg k-h
> >
> > --
> > 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+unsubscr...@googlegroups.com.
> > To view this discussion on the web visit 
> > https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQN%40kroah.com.
> >
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Greg KH wrote:

> On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > it is used everywhere as a bool and, accordingly, it should be
> > declared as a bool. Shorten the controlling
> > expression of an 'if' statement.
> >
> > Signed-off-by: Fabio M. De Francesco 
> > ---
> >  drivers/staging/rtl8723bs/hal/hal_intf.c| 2 +-
> >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c 
> > b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > index 96fe172ced8d..8dc4dd8c6d4c 100644
> > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
> >
> >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> >  {
> > -   if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
> > +   if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* 
> > this function caller is in interrupt context */
> > }
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h 
> > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > index 0a48f1653be5..0767dbb84199 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > u8 LpsIdleCount;
> > u8 power_mgnt;
> > u8 org_power_mgnt;
> > -   u8 fw_current_in_ps_mode;
> > +   bool fw_current_in_ps_mode;
> > unsigned long   DelayLPSLastTimeStamp;
> > s32 pnp_current_pwr_state;
> > u8 pnp_bstop_trx;
>
> If this is only checked, how can it ever be true?  Who ever sets this
> value?

I think it's already updated everywhere with true and false, so there is
nothing to change.  But it would be good to make that clear in the log
message.

julia

>
> thanks,
>
> greg k-h
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQN%40kroah.com.
>

Re: [Outreachy kernel] [Patch 0/3]

[Julia Lawall]

On Sat, 10 Apr 2021, Greg KH wrote:

> On Sat, Apr 10, 2021 at 09:57:00AM +0200, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 9:29:29 AM CEST Greg KH wrote:
> > > On Fri, Apr 09, 2021 at 06:29:59PM +0200, Fabio M. De Francesco wrote:
> > > > This patch series removes camelcases, changes the type and use of a
> > > > variable, and correct misspelled words.
> > > >
> > > > Patch 1/3: staging: rtl8723bs: Remove camelcase in several files
> > > >
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c   |  2 +-
> > > > drivers/staging/rtl8723bs/core/rtw_mlme.c  |  2 +-
> > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c   | 18 +-
> > > > drivers/staging/rtl8723bs/hal/hal_intf.c   |  2 +-
> > > > drivers/staging/rtl8723bs/hal/rtl8723b_dm.c|  6 +++---
> > > > .../staging/rtl8723bs/hal/rtl8723b_hal_init.c  |  2 +-
> > > > drivers/staging/rtl8723bs/hal/sdio_ops.c   | 14 +++---
> > > > .../staging/rtl8723bs/include/rtw_pwrctrl.h|  2 +-
> > > > 8 files changed, 24 insertions(+), 24 deletions(-)
> > > >
> > > > Patch 2/3: staging: rtl8723bs: Change the type and use of a variable
> > > >
> > > > drivers/staging/rtl8723bs/hal/hal_intf.c| 2 +-
> > > >
> > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > Patch 3/3: staging: rtl8723bs: include: Fix misspelled words in
> > > > comments
> > > >
> > > > .../rtl8723bs/include/Hal8192CPhyReg.h|  8 ++---
> > > >
> > > >  .../staging/rtl8723bs/include/basic_types.h   |  2 +-
> > > >  drivers/staging/rtl8723bs/include/drv_types.h |  2 +-
> > > >  drivers/staging/rtl8723bs/include/hal_com.h   |  2 +-
> > > >  .../staging/rtl8723bs/include/hal_com_reg.h   | 34 +--
> > > >  drivers/staging/rtl8723bs/include/hal_data.h  |  2 +-
> > > >  .../staging/rtl8723bs/include/hal_pwr_seq.h   |  2 +-
> > > >  drivers/staging/rtl8723bs/include/rtw_cmd.h   |  6 ++--
> > > >  drivers/staging/rtl8723bs/include/rtw_mlme.h  | 18 +-
> > > >  .../staging/rtl8723bs/include/rtw_mlme_ext.h  |  2 +-
> > > >  drivers/staging/rtl8723bs/include/rtw_mp.h|  2 +-
> > > >  .../staging/rtl8723bs/include/rtw_pwrctrl.h   |  2 +-
> > > >  drivers/staging/rtl8723bs/include/rtw_recv.h  |  4 +--
> > > >  drivers/staging/rtl8723bs/include/rtw_xmit.h  |  2 +-
> > > >  drivers/staging/rtl8723bs/include/sta_info.h  |  2 +-
> > > >  drivers/staging/rtl8723bs/include/wifi.h  |  2 +-
> > > >  16 files changed, 46 insertions(+), 46 deletions(-)
> > > >
> > > > Fabio M. De Francesco
> > >
> > > I have no idea how you created this patch series, but I do not think it
> > > worked well :(
> > >
> > > Please use our standard tools, that do this all for you automatically,
> > >
> > Apart from the fact that I forgot to save the subject, I didn't know that
> > there are standard tools for doing Patch 0/N. Therefore I thought to make
> > an email message that contained the information about the patch series.
> >
> > I was wrong, sorry. I have to research this topic in the git manual or
> > somewhere else.
>
> 'git format-patch --cover-letter main..HEAD' will to the trick, if you
> are on a different branch from 'main'.  Then edit the -* file and
> then send them all off with 'git send-email'.

Fabio,

The --cover-letter option is discussed in the "Versioning patchsets"
section of the tutorial.  Perhaps that is not the best place for it, since
the need for a cover letter is not specific to versioning...

julia

>
> thanks,
>
> greg k-h
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHFcI104K%2Bih3UDG%40kroah.com.
>

Re: [Outreachy kernel] [Patch 0/3]

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 9:29:29 AM CEST Greg KH wrote:
> > On Fri, Apr 09, 2021 at 06:29:59PM +0200, Fabio M. De Francesco wrote:
> > > This patch series removes camelcases, changes the type and use of a
> > > variable, and correct misspelled words.
> > >
> > > Patch 1/3: staging: rtl8723bs: Remove camelcase in several files
> > >
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c   |  2 +-
> > > drivers/staging/rtl8723bs/core/rtw_mlme.c  |  2 +-
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c   | 18 +-
> > > drivers/staging/rtl8723bs/hal/hal_intf.c   |  2 +-
> > > drivers/staging/rtl8723bs/hal/rtl8723b_dm.c|  6 +++---
> > > .../staging/rtl8723bs/hal/rtl8723b_hal_init.c  |  2 +-
> > > drivers/staging/rtl8723bs/hal/sdio_ops.c   | 14 +++---
> > > .../staging/rtl8723bs/include/rtw_pwrctrl.h|  2 +-
> > > 8 files changed, 24 insertions(+), 24 deletions(-)
> > >
> > > Patch 2/3: staging: rtl8723bs: Change the type and use of a variable
> > >
> > > drivers/staging/rtl8723bs/hal/hal_intf.c| 2 +-
> > >
> > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > Patch 3/3: staging: rtl8723bs: include: Fix misspelled words in
> > > comments
> > >
> > > .../rtl8723bs/include/Hal8192CPhyReg.h|  8 ++---
> > >
> > >  .../staging/rtl8723bs/include/basic_types.h   |  2 +-
> > >  drivers/staging/rtl8723bs/include/drv_types.h |  2 +-
> > >  drivers/staging/rtl8723bs/include/hal_com.h   |  2 +-
> > >  .../staging/rtl8723bs/include/hal_com_reg.h   | 34 +--
> > >  drivers/staging/rtl8723bs/include/hal_data.h  |  2 +-
> > >  .../staging/rtl8723bs/include/hal_pwr_seq.h   |  2 +-
> > >  drivers/staging/rtl8723bs/include/rtw_cmd.h   |  6 ++--
> > >  drivers/staging/rtl8723bs/include/rtw_mlme.h  | 18 +-
> > >  .../staging/rtl8723bs/include/rtw_mlme_ext.h  |  2 +-
> > >  drivers/staging/rtl8723bs/include/rtw_mp.h|  2 +-
> > >  .../staging/rtl8723bs/include/rtw_pwrctrl.h   |  2 +-
> > >  drivers/staging/rtl8723bs/include/rtw_recv.h  |  4 +--
> > >  drivers/staging/rtl8723bs/include/rtw_xmit.h  |  2 +-
> > >  drivers/staging/rtl8723bs/include/sta_info.h  |  2 +-
> > >  drivers/staging/rtl8723bs/include/wifi.h  |  2 +-
> > >  16 files changed, 46 insertions(+), 46 deletions(-)
> > >
> > > Fabio M. De Francesco
> >
> > I have no idea how you created this patch series, but I do not think it
> > worked well :(
> >
> > Please use our standard tools, that do this all for you automatically,
> >
> Apart from the fact that I forgot to save the subject, I didn't know that
> there are standard tools for doing Patch 0/N. Therefore I thought to make
> an email message that contained the information about the patch series.
>
> I was wrong, sorry. I have to research this topic in the git manual or
> somewhere else.

Everything should be clear in the Kernelnewbies outreachy tutorial.  If
there is something about this issue that is not clear, please comment on
it or propose a fix.

julia


>
> Thanks,
>
> Fabio
> >
> > and send them out in a series of emails that have a proper subject line
> > (look at what you used here...) and are properly "threaded".
> >
> > thanks,
> >
> > greg k-h
>
>
>
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/2101069.1PIH1tPmJ7%40localhost.localdomain.
>

Re: [Outreachy kernel] [PATCH 1/3] staging: rtl8192e: replace comparison to NULL by boolean

[Julia Lawall]

On Sat, 10 Apr 2021, Mitali Borkar wrote:

> Replaced comparison to NULL by boolean expressions
> (here used boolean negations). This improves readability of code.
> Reported by checkpatch.
>
> Signed-off-by: Mitali Borkar 
> ---
>  drivers/staging/rtl8192e/rtl819x_HTProc.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c 
> b/drivers/staging/rtl8192e/rtl819x_HTProc.c
> index 65202dd53447..0b1e92f17805 100644
> --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c
> @@ -276,7 +276,7 @@ void HTConstructCapabilityElement(struct rtllib_device 
> *ieee, u8 *posHTCap,
>   struct rt_hi_throughput *pHT = ieee->pHTInfo;
>   struct ht_capab_ele *pCapELE = NULL;
>
> - if ((posHTCap == NULL) || (pHT == NULL)) {
> + if ((!posHTCap) || (!pHT)) {

You can drop the parentheses. ! has higher precedenace than ||.

julia

>   netdev_warn(ieee->dev,
>   "%s(): posHTCap and pHTInfo are null\n", __func__);
>   return;
> @@ -357,7 +357,7 @@ void HTConstructInfoElement(struct rtllib_device *ieee, 
> u8 *posHTInfo,
>   struct rt_hi_throughput *pHT = ieee->pHTInfo;
>   struct ht_info_ele *pHTInfoEle = (struct ht_info_ele *)posHTInfo;
>
> - if ((posHTInfo == NULL) || (pHTInfoEle == NULL)) {
> + if ((!posHTInfo) || (!pHTInfoEle)) {
>   netdev_warn(ieee->dev,
>   "%s(): posHTInfo and pHTInfoEle are null\n",
>   __func__);
> @@ -397,7 +397,7 @@ void HTConstructInfoElement(struct rtllib_device *ieee, 
> u8 *posHTInfo,
>  void HTConstructRT2RTAggElement(struct rtllib_device *ieee, u8 *posRT2RTAgg,
>   u8 *len)
>  {
> - if (posRT2RTAgg == NULL) {
> + if (!posRT2RTAgg) {
>   netdev_warn(ieee->dev, "%s(): posRT2RTAgg is null\n", __func__);
>   return;
>   }
> @@ -420,7 +420,7 @@ static u8 HT_PickMCSRate(struct rtllib_device *ieee, u8 
> *pOperateMCS)
>  {
>   u8 i;
>
> - if (pOperateMCS == NULL) {
> + if (!pOperateMCS) {
>   netdev_warn(ieee->dev, "%s(): pOperateMCS is null\n", __func__);
>   return false;
>   }
> @@ -453,7 +453,7 @@ u8 HTGetHighestMCSRate(struct rtllib_device *ieee, u8 
> *pMCSRateSet,
>   u8  mcsRate = 0;
>   u8  availableMcsRate[16];
>
> - if (pMCSRateSet == NULL || pMCSFilter == NULL) {
> + if (!pMCSRateSet || !pMCSFilter) {
>   netdev_warn(ieee->dev,
>   "%s(): pMCSRateSet and pMCSFilter are null\n",
>   __func__);
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHEJngq5MHBEspGY%40kali.
>

Re: [Outreachy kernel] [PATCH v2 1/2] media: zoran: add spaces around '<<'

[Julia Lawall]

On Sat, 10 Apr 2021, Mitali Borkar wrote:

> No changes required in this patch.
> In v1:- Added spaces around '<<' operator to improve readability and meet 
> linux kernel coding
> style

The text above would go in the git history.  "No changes required in this
patch." doesn't make sense in that context.  If you want to say something
that relates to the history of the submitted patches, then that should be
under the ---.  The text there will disappear when the patch is applied.

julia


>
> Signed-off-by: Mitali Borkar 
> ---
>
> Changes from v1:- No changes required in this patch. Below is the git
> diff of v1.
>
>  drivers/staging/media/zoran/zr36057.h | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/media/zoran/zr36057.h 
> b/drivers/staging/media/zoran/zr36057.h
> index 71b651add35a..a2a75fd9f535 100644
> --- a/drivers/staging/media/zoran/zr36057.h
> +++ b/drivers/staging/media/zoran/zr36057.h
> @@ -30,13 +30,13 @@
>  #define ZR36057_VFESPFR_HOR_DCM  14
>  #define ZR36057_VFESPFR_VER_DCM  8
>  #define ZR36057_VFESPFR_DISP_MODE6
> -#define ZR36057_VFESPFR_YUV422  (0<<3)
> -#define ZR36057_VFESPFR_RGB888  (1<<3)
> -#define ZR36057_VFESPFR_RGB565  (2<<3)
> -#define ZR36057_VFESPFR_RGB555  (3<<3)
> -#define ZR36057_VFESPFR_ERR_DIF  (1<<2)
> -#define ZR36057_VFESPFR_PACK24  (1<<1)
> -#define ZR36057_VFESPFR_LITTLE_ENDIAN(1<<0)
> +#define ZR36057_VFESPFR_YUV422  (0 << 3)
> +#define ZR36057_VFESPFR_RGB888  (1 << 3)
> +#define ZR36057_VFESPFR_RGB565  (2 << 3)
> +#define ZR36057_VFESPFR_RGB555  (3 << 3)
> +#define ZR36057_VFESPFR_ERR_DIF  (1 << 2)
> +#define ZR36057_VFESPFR_PACK24  (1 << 1)
> +#define ZR36057_VFESPFR_LITTLE_ENDIAN(1 << 0)
>
>  #define ZR36057_VDTR0x00c/* Video Display "Top" Register 
> */
>
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHCgksbiLv0pFF2F%40kali.
>

Re: [Outreachy kernel] [PATCH v2 2/2] staging: media: zoran: remove and add comments; align code

[Julia Lawall]

On Sat, 10 Apr 2021, Mitali Borkar wrote:

> Removed comments from the same line and added them to new line above the
> blocks, aligned everything properly by using tabs to make code neater
> and improve readability.
>
> Signed-off-by: Mitali Borkar 
> ---
>  drivers/staging/media/zoran/zr36057.h | 293 ++
>  1 file changed, 162 insertions(+), 131 deletions(-)
>
> diff --git a/drivers/staging/media/zoran/zr36057.h 
> b/drivers/staging/media/zoran/zr36057.h
> index 93075459f910..198d344a8879 100644
> --- a/drivers/staging/media/zoran/zr36057.h
> +++ b/drivers/staging/media/zoran/zr36057.h
> @@ -12,145 +12,176 @@
>
>  /* Zoran ZR36057 registers */
>
> -#define ZR36057_VFEHCR  0x000/* Video Front End, Horizontal 
> Configuration Register */
> -#define ZR36057_VFEHCR_HS_POL BIT(30)
> -#define ZR36057_VFEHCR_H_START   10
> +/* Video Front End, Horizontal Configuration Register */
> +#define ZR36057_VFEHCR   0x000
> +#define ZR36057_VFEHCR_HS_POLBIT(30)

It looks like the alignment didn't work out here?  Check that the use of
tabs is the same as on the nearby lines.

> +#define ZR36057_VFEHCR_H_START   10
>  #define ZR36057_VFEHCR_H_END 0
>  #define ZR36057_VFEHCR_HMASK 0x3ff
>
> -#define ZR36057_VFEVCR  0x004/* Video Front End, Vertical 
> Configuration Register */
> -#define ZR36057_VFEVCR_VS_POL BIT(30)
> -#define ZR36057_VFEVCR_V_START   10
> +/* Video Front End, Vertical Configuration Register */
> +#define ZR36057_VFEVCR   0x004
> +#define ZR36057_VFEVCR_VS_POLBIT(30)
> +#define ZR36057_VFEVCR_V_START   10
>  #define ZR36057_VFEVCR_V_END 0
>  #define ZR36057_VFEVCR_VMASK 0x3ff
>
> -#define ZR36057_VFESPFR 0x008/* Video Front End, Scaler and 
> Pixel Format Register */
> -#define ZR36057_VFESPFR_EXT_FLBIT(26)
> -#define ZR36057_VFESPFR_TOP_FIELD BIT(25)
> -#define ZR36057_VFESPFR_VCLK_POL  BIT(24)
> -#define ZR36057_VFESPFR_H_FILTER 21
> -#define ZR36057_VFESPFR_HOR_DCM  14
> -#define ZR36057_VFESPFR_VER_DCM  8
> -#define ZR36057_VFESPFR_DISP_MODE6
> -#define ZR36057_VFESPFR_YUV422  (0 << 3)
> -#define ZR36057_VFESPFR_RGB888  BIT(3)
> -#define ZR36057_VFESPFR_RGB565  (2 << 3)
> -#define ZR36057_VFESPFR_RGB555  (3 << 3)
> -#define ZR36057_VFESPFR_ERR_DIF  BIT(2)
> -#define ZR36057_VFESPFR_PACK24  BIT(1)
> -#define ZR36057_VFESPFR_LITTLE_ENDIANBIT(0)
> -
> -#define ZR36057_VDTR0x00c/* Video Display "Top" Register 
> */
> -
> -#define ZR36057_VDBR0x010/* Video Display "Bottom" 
> Register */
> -
> -#define ZR36057_VSSFGR  0x014/* Video Stride, Status, and 
> Frame Grab Register */
> -#define ZR36057_VSSFGR_DISP_STRIDE   16
> -#define ZR36057_VSSFGR_VID_OVFBIT(8)
> -#define ZR36057_VSSFGR_SNAP_SHOT  BIT(1)
> -#define ZR36057_VSSFGR_FRAME_GRAB BIT(0)
> -
> -#define ZR36057_VDCR0x018/* Video Display Configuration 
> Register */
> -#define ZR36057_VDCR_VID_EN   BIT(31)
> -#define ZR36057_VDCR_MIN_PIX 24
> -#define ZR36057_VDCR_TRITON  BIT(24)
> -#define ZR36057_VDCR_VID_WIN_HT   12
> -#define ZR36057_VDCR_VID_WIN_WID  0
> -
> -#define ZR36057_MMTR0x01c/* Masking Map "Top" Register */
> -
> -#define ZR36057_MMBR0x020/* Masking Map "Bottom" 
> Register */
> -
> -#define ZR36057_OCR 0x024/* Overlay Control Register */
> -#define ZR36057_OCR_OVL_ENABLEBIT(15)
> -#define ZR36057_OCR_MASK_STRIDE  0
> -
> -#define ZR36057_SPGPPCR 0x028/* System, PCI, and General 
> Purpose Pins Control Register */
> -#define ZR36057_SPGPPCR_SOFT_RESETBIT(24)
> -
> -#define ZR36057_GPPGCR1 0x02c/* General Purpose Pins and 
> GuestBus Control Register (1) */
> -
> -#define ZR36057_MCSAR   0x030/* MPEG Code Source Address 
> Register */
> -
> -#define ZR36057_MCTCR   0x034/* MPEG Code Transfer Control 
> Register */
> -#define ZR36057_MCTCR_COD_TIMEBIT(30)
> -#define ZR36057_MCTCR_C_EMPTY BIT(29)
> -#define ZR36057_MCTCR_C_FLUSH BIT(28)
> +/* Video Front End, Scaler and Pixel Format Register */
> +#define ZR36057_VFESPFR  0x008
> +#define ZR36057_VFESPFR_EXT_FL   BIT(26)
> +#define ZR36057_VFESPFR_TOP_FIELDBIT(25)
> +#define ZR36057_VFESPFR_VCLK_POL BIT(24)
> +#define ZR36057_VFESPFR_H_FILTER 21
> +#define ZR36057_VFESPFR_HOR_DCM  14
> +#define ZR36057_VFESPFR_VER_DCM  8
> +#define ZR36057_VFESPFR_DISP_MODE6

The above four lines also look odd.

Re: [Outreachy kernel] Re: [PATCH 1/2 v2] staging: media: hantro: Align line break to the open parenthesis in file hantro_hw.h

[Julia Lawall]

On Thu, 8 Apr 2021, Ezequiel Garcia wrote:

> Ola Aline,
>
> Welcome to the kernel community. Hope you enjoy some of this
> Outreachy adventures.
>
> Normally, when you submit a v2, we want to know what changed
> between the first submission and v2.
>
> If you are subscribed to linux-media, you can read some
> of the series with a vN+1 and look how it's done. Examples:
>
> https://www.spinics.net/lists/linux-media/msg190043.html
>
> https://www.spinics.net/lists/linux-media/msg189923.html
>
> I'm sure your Outreachy mentors can tell you more.

There is information in the kernelnewbies tutorial about how to resubmit
patches.

julia

>
> On Thu, 2021-04-08 at 11:07 -0300, Aline Santana Cordeiro wrote:
> > Aligns line break with the remaining function arguments
> > to the open parenthesis. Issue found by checkpatch.
> >
> > Signed-off-by: Aline Santana Cordeiro 
> > ---
> >  drivers/staging/media/hantro/hantro_hw.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h 
> > b/drivers/staging/media/hantro/hantro_hw.h
> > index 34c9e46..a650b9c 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -207,7 +207,7 @@ hantro_h264_mv_size(unsigned int width, unsigned int 
> > height)
> >  void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx);
> >  void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx);
> >  void hantro_mpeg2_dec_copy_qtable(u8 *qtable,
> > -   const struct v4l2_ctrl_mpeg2_quantization *ctrl);
> > + const struct v4l2_ctrl_mpeg2_quantization 
> > *ctrl);
> >  int hantro_mpeg2_dec_init(struct hantro_ctx *ctx);
> >  void hantro_mpeg2_dec_exit(struct hantro_ctx *ctx);
> >  
>
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/d4000598fddcf45e578462aa04b556c0ca057091.camel%40collabora.com.
>

Re: [Outreachy kernel] Re: [PATCH 1/2] media: zoran: add spaces around '<<'

[Julia Lawall]

On Fri, 9 Apr 2021, Mitali Borkar wrote:

> On Fri, Apr 09, 2021 at 09:23:22AM +0200, Hans Verkuil wrote:
> > Hi Mitali,
> >
> > On 08/04/2021 22:38, Mitali Borkar wrote:
> > > Added spaces around '<<' operator to improve readability and meet linux
> > > kernel coding style.
> > > Reported by checkpatch
> > >
> > > Signed-off-by: Mitali Borkar 
> > > ---
> > >  drivers/staging/media/zoran/zr36057.h | 14 +++---
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/zoran/zr36057.h 
> > > b/drivers/staging/media/zoran/zr36057.h
> > > index 71b651add35a..a2a75fd9f535 100644
> > > --- a/drivers/staging/media/zoran/zr36057.h
> > > +++ b/drivers/staging/media/zoran/zr36057.h
> > > @@ -30,13 +30,13 @@
> > >  #define ZR36057_VFESPFR_HOR_DCM  14
> > >  #define ZR36057_VFESPFR_VER_DCM  8
> > >  #define ZR36057_VFESPFR_DISP_MODE6
> > > -#define ZR36057_VFESPFR_YUV422  (0<<3)
> > > -#define ZR36057_VFESPFR_RGB888  (1<<3)
> > > -#define ZR36057_VFESPFR_RGB565  (2<<3)
> > > -#define ZR36057_VFESPFR_RGB555  (3<<3)
> > > -#define ZR36057_VFESPFR_ERR_DIF  (1<<2)
> > > -#define ZR36057_VFESPFR_PACK24  (1<<1)
> > > -#define ZR36057_VFESPFR_LITTLE_ENDIAN(1<<0)
> > > +#define ZR36057_VFESPFR_YUV422  (0 << 3)
> > > +#define ZR36057_VFESPFR_RGB888  (1 << 3)
> > > +#define ZR36057_VFESPFR_RGB565  (2 << 3)
> > > +#define ZR36057_VFESPFR_RGB555  (3 << 3)
> > > +#define ZR36057_VFESPFR_ERR_DIF  (1 << 2)
> > > +#define ZR36057_VFESPFR_PACK24  (1 << 1)
> > > +#define ZR36057_VFESPFR_LITTLE_ENDIAN(1 << 0)
> > >
> > >  #define ZR36057_VDTR0x00c/* Video Display "Top" Register 
> > > */
> > >
> > >
> >
> > I looked at that header and it is very messy.
> >
> > Can you make two new patches? The first aligns every nicely, e.g. this:
> >
> > #define ZR36057_VFEHCR  0x000   /* Video Front End, Horizontal 
> > Configuration Register */
> > #define ZR36057_VFEHCR_HS_POL BIT(30)
> > #define ZR36057_VFEHCR_H_START   10
> > #define ZR36057_VFEHCR_H_END0
> > #define ZR36057_VFEHCR_HMASK0x3ff
> >
> > should become:
> >
> > /* Video Front End, Horizontal Configuration Register */
> > #define ZR36057_VFEHCR  0x000
> > #define ZR36057_VFEHCR_HS_POL   BIT(30)
> > #define ZR36057_VFEHCR_H_START  10
> > #define ZR36057_VFEHCR_H_END0
> > #define ZR36057_VFEHCR_HMASK0x3ff
> >
> > Same for all the other register blocks. Use tabs to do the alignment
> > instead of spaces, as is currently the case.
> >
> > The second patch can replace the (0<<3) etc. to BIT(0).
> >
> Then I guess only one new patch would be needed for proper alignment, am
> i right? I have to rename it as v2 or should send as a completely new
> patch?

v2 might reduce confusion.

julia

> > That would be a nice cleanup of this rather messy header.
> >
> > Thanks!
> >
> > Hans
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHCFsNZVGfjjyHDi%40kali.
>

Re: [Outreachy kernel][PATCH] staging: media: hantro: Rewrite macro function in lower case

[Julia Lawall]

On Fri, 9 Apr 2021, ascordeiro wrote:

> Em sex, 2021-04-09 às 13:44 -0300, Ezequiel Garcia escreveu:
> > Hi Aline,
>
> Hi Ezequiel,
> >
> > On Fri, 2021-04-09 at 10:54 -0300, Aline Santana Cordeiro wrote:
> > > Rewrite macros resembling functions #define HANTRO_PP_REG_WRITE
> > > and #define HANTRO_PP_RED_WRITE_S in lower case, according with
> > > code style.

Maybe you can see if these macros can be converted to static inline
functions.  Macros don't provide any type checking.

julia

> > >
> >
> > Where is this written in the Coding Style?
>
> I found this in section 12, about Macros, Enums and RTL in both
> references:
> https://www.kernel.org/doc/html/latest/process/coding-style.html
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/coding-style.rst
> >
> > Thanks!
> > Ezequiel
>
> Thank you!
> Aline
> >
> > > Signed-off-by: Aline Santana Cordeiro <
> > > alinesantanacorde...@gmail.com>
> > > ---
> > >  drivers/staging/media/hantro/hantro_postproc.c | 34 +-
> > > 
> > >  1 file changed, 17 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro_postproc.c
> > > b/drivers/staging/media/hantro/hantro_postproc.c
> > > index 6d2a8f2a..06279c0 100644
> > > --- a/drivers/staging/media/hantro/hantro_postproc.c
> > > +++ b/drivers/staging/media/hantro/hantro_postproc.c
> > > @@ -12,14 +12,14 @@
> > >  #include "hantro_hw.h"
> > >  #include "hantro_g1_regs.h"
> > >  
> > > -#define HANTRO_PP_REG_WRITE(vpu, reg_name, val) \
> > > +#define hantro_pp_reg_write(vpu, reg_name, val) \
> > >  { \
> > > hantro_reg_write(vpu, \
> > >  &(vpu)->variant->postproc_regs->reg_name,
> > > \
> > >  val); \
> > >  }
> > >  
> > > -#define HANTRO_PP_REG_WRITE_S(vpu, reg_name, val) \
> > > +#define hantro_pp_reg_write_s(vpu, reg_name, val) \
> > >  { \
> > > hantro_reg_write_s(vpu, \
> > >    &(vpu)->variant->postproc_regs-
> > > >reg_name, \
> > > @@ -61,7 +61,7 @@ void hantro_postproc_enable(struct hantro_ctx
> > > *ctx)
> > > return;
> > >  
> > > /* Turn on pipeline mode. Must be done first. */
> > > -   HANTRO_PP_REG_WRITE_S(vpu, pipeline_en, 0x1);
> > > +   hantro_pp_reg_write_s(vpu, pipeline_en, 0x1);
> > >  
> > > src_pp_fmt = VPU_PP_IN_NV12;
> > >  
> > > @@ -79,19 +79,19 @@ void hantro_postproc_enable(struct hantro_ctx
> > > *ctx)
> > > dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > > dst_dma = vb2_dma_contig_plane_dma_addr(_buf->vb2_buf,
> > > 0);
> > >  
> > > -   HANTRO_PP_REG_WRITE(vpu, clk_gate, 0x1);
> > > -   HANTRO_PP_REG_WRITE(vpu, out_endian, 0x1);
> > > -   HANTRO_PP_REG_WRITE(vpu, out_swap32, 0x1);
> > > -   HANTRO_PP_REG_WRITE(vpu, max_burst, 16);
> > > -   HANTRO_PP_REG_WRITE(vpu, out_luma_base, dst_dma);
> > > -   HANTRO_PP_REG_WRITE(vpu, input_width, MB_WIDTH(ctx-
> > > >dst_fmt.width));
> > > -   HANTRO_PP_REG_WRITE(vpu, input_height, MB_HEIGHT(ctx-
> > > >dst_fmt.height));
> > > -   HANTRO_PP_REG_WRITE(vpu, input_fmt, src_pp_fmt);
> > > -   HANTRO_PP_REG_WRITE(vpu, output_fmt, dst_pp_fmt);
> > > -   HANTRO_PP_REG_WRITE(vpu, output_width, ctx->dst_fmt.width);
> > > -   HANTRO_PP_REG_WRITE(vpu, output_height, ctx-
> > > >dst_fmt.height);
> > > -   HANTRO_PP_REG_WRITE(vpu, orig_width, MB_WIDTH(ctx-
> > > >dst_fmt.width));
> > > -   HANTRO_PP_REG_WRITE(vpu, display_width, ctx-
> > > >dst_fmt.width);
> > > +   hantro_pp_reg_write(vpu, clk_gate, 0x1);
> > > +   hantro_pp_reg_write(vpu, out_endian, 0x1);
> > > +   hantro_pp_reg_write(vpu, out_swap32, 0x1);
> > > +   hantro_pp_reg_write(vpu, max_burst, 16);
> > > +   hantro_pp_reg_write(vpu, out_luma_base, dst_dma);
> > > +   hantro_pp_reg_write(vpu, input_width, MB_WIDTH(ctx-
> > > >dst_fmt.width));
> > > +   hantro_pp_reg_write(vpu, input_height, MB_HEIGHT(ctx-
> > > >dst_fmt.height));
> > > +   hantro_pp_reg_write(vpu, input_fmt, src_pp_fmt);
> > > +   hantro_pp_reg_write(vpu, output_fmt, dst_pp_fmt);
> > > +   hantro_pp_reg_write(vpu, output_width, ctx->dst_fmt.width);
> > > +   hantro_pp_reg_write(vpu, output_height, ctx-
> > > >dst_fmt.height);
> > > +   hantro_pp_reg_write(vpu, orig_width, MB_WIDTH(ctx-
> > > >dst_fmt.width));
> > > +   hantro_pp_reg_write(vpu, display_width, ctx-
> > > >dst_fmt.width);
> > >  }
> > >  
> > >  void hantro_postproc_free(struct hantro_ctx *ctx)
> > > @@ -146,5 +146,5 @@ void hantro_postproc_disable(struct hantro_ctx
> > > *ctx)
> > > if (!vpu->variant->postproc_regs)
> > > return;
> > >  
> > > -   HANTRO_PP_REG_WRITE_S(vpu, pipeline_en, 0x0);
> > > +   hantro_pp_reg_write_s(vpu, pipeline_en, 0x0);
> > >  }
> >
> >
>
>
> --
> You received this message because you are subscribed to the Google Groups 
> "outreachy-kernel" group.
> To unsubscribe from this 

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Fri, 9 Apr 2021, Fabio M. De Francesco wrote:

> On Friday, April 9, 2021 4:12:37 PM CEST Greg KH wrote:
> > On Thu, Apr 08, 2021 at 01:19:42PM +0200, Fabio M. De Francesco wrote:
> > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > it is used everywhere as a bool and, accordingly, it should be
> > > declared as a bool. Shorten the controlling
> > > expression of an 'if' statement.
> > >
> > > Signed-off-by: Fabio M. De Francesco 
> > > ---
> > >
> > >  drivers/staging/rtl8723bs/hal/hal_intf.c| 2 +-
> > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > I now have 3 patches, I think, for this same driver, from you, and I
> > have no idea what order they should be applied in.
> >
> > So I'm going to drop them all.  Can you please resend me a patch series,
> > with all of the outstanding patches sent to me from you that I have not
> > applied yet, so that I know which ones to look at and what order to
> > apply them in?
> >
> I just sent in the series of patches you requested. Hope the work is as you
> expected.

If you make multiple patches that touch the same files they have to be in
a series.

julia

Re: [Outreachy kernel] Re: [PATCH 1/2] media: zoran: add spaces around '<<'

[Julia Lawall]

On Fri, 9 Apr 2021, Mitali Borkar wrote:

> On Fri, Apr 09, 2021 at 09:23:22AM +0200, Hans Verkuil wrote:
> > Hi Mitali,
> >
> > On 08/04/2021 22:38, Mitali Borkar wrote:
> > > Added spaces around '<<' operator to improve readability and meet linux
> > > kernel coding style.
> > > Reported by checkpatch
> > >
> > > Signed-off-by: Mitali Borkar 
> > > ---
> > >  drivers/staging/media/zoran/zr36057.h | 14 +++---
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/zoran/zr36057.h 
> > > b/drivers/staging/media/zoran/zr36057.h
> > > index 71b651add35a..a2a75fd9f535 100644
> > > --- a/drivers/staging/media/zoran/zr36057.h
> > > +++ b/drivers/staging/media/zoran/zr36057.h
> > > @@ -30,13 +30,13 @@
> > >  #define ZR36057_VFESPFR_HOR_DCM  14
> > >  #define ZR36057_VFESPFR_VER_DCM  8
> > >  #define ZR36057_VFESPFR_DISP_MODE6
> > > -#define ZR36057_VFESPFR_YUV422  (0<<3)
> > > -#define ZR36057_VFESPFR_RGB888  (1<<3)
> > > -#define ZR36057_VFESPFR_RGB565  (2<<3)
> > > -#define ZR36057_VFESPFR_RGB555  (3<<3)
> > > -#define ZR36057_VFESPFR_ERR_DIF  (1<<2)
> > > -#define ZR36057_VFESPFR_PACK24  (1<<1)
> > > -#define ZR36057_VFESPFR_LITTLE_ENDIAN(1<<0)
> > > +#define ZR36057_VFESPFR_YUV422  (0 << 3)
> > > +#define ZR36057_VFESPFR_RGB888  (1 << 3)
> > > +#define ZR36057_VFESPFR_RGB565  (2 << 3)
> > > +#define ZR36057_VFESPFR_RGB555  (3 << 3)
> > > +#define ZR36057_VFESPFR_ERR_DIF  (1 << 2)
> > > +#define ZR36057_VFESPFR_PACK24  (1 << 1)
> > > +#define ZR36057_VFESPFR_LITTLE_ENDIAN(1 << 0)
> > >
> > >  #define ZR36057_VDTR0x00c/* Video Display "Top" Register 
> > > */
> > >
> > >
> >
> > I looked at that header and it is very messy.
> >
> > Can you make two new patches? The first aligns every nicely, e.g. this:
> >
> > #define ZR36057_VFEHCR  0x000   /* Video Front End, Horizontal 
> > Configuration Register */
> > #define ZR36057_VFEHCR_HS_POL BIT(30)
> > #define ZR36057_VFEHCR_H_START   10
> > #define ZR36057_VFEHCR_H_END0
> > #define ZR36057_VFEHCR_HMASK0x3ff
> >
> > should become:
> >
> > /* Video Front End, Horizontal Configuration Register */
> > #define ZR36057_VFEHCR  0x000
> > #define ZR36057_VFEHCR_HS_POL   BIT(30)
> > #define ZR36057_VFEHCR_H_START  10
> > #define ZR36057_VFEHCR_H_END0
> > #define ZR36057_VFEHCR_HMASK0x3ff
> >
> > Same for all the other register blocks. Use tabs to do the alignment
> > instead of spaces, as is currently the case.
> >
> Okay Sir, will do this.
>
> > The second patch can replace the (0<<3) etc. to BIT(0).
> >
> Sir may I know how to write (2<<3) in BIT() form? Like I am still
> learning and not sure how to convert this. Should it be BIT(2)? But this
> is only possible for (1<
> Thanks.
>
> > That would be a nice cleanup of this rather messy header.
> >
> > Thanks!
> >
> > Hans
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YHBukmdxSiRc%2BK6I%40kali.
>

Re: [Outreachy kernel] [PATCH v6] staging: rtl8723bs: Remove camelcase in several files

[Julia Lawall]

On Fri, 9 Apr 2021, Fabio M. De Francesco wrote:

> Remove camelcase in bFwCurrentInPSMode, a variable used by code
> of several subdirectories/files of the driver. Issue detected by
> checkpatch.pl. Delete the unnecessary "b" (that stands for "byte") from
> the beginning of the name.

I'm sorry, but this is still the wrong patch.  Now you have b in the
starting point and b_ in the ending point.

I would really suggest to start with a competely fresh Linux kernel, redo
the patch by hand, and then send that.

julia

>
> Signed-off-by: Fabio M. De Francesco 
> ---
>
> Changes from v5: Edit against the wrong patch
> Changes from v4: Mention the removal of the initial "b" in log message.
> Changes from v3: Fix errors in the format of the patch.
> Changes from v2: Remove unnecessary comment. Shortened a function name.
> Changes from v1: No changes to the code but only to the subject for the
> purpose to differentiate this patch because other removes of camelcase
> have been made in other files of the same directory.
>
>  drivers/staging/rtl8723bs/core/rtw_cmd.c   |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_mlme.c  |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_pwrctrl.c   | 18 +-
>  drivers/staging/rtl8723bs/hal/hal_intf.c   |  2 +-
>  drivers/staging/rtl8723bs/hal/rtl8723b_dm.c|  6 +++---
>  .../staging/rtl8723bs/hal/rtl8723b_hal_init.c  |  2 +-
>  drivers/staging/rtl8723bs/hal/sdio_ops.c   | 14 +++---
>  .../staging/rtl8723bs/include/rtw_pwrctrl.h|  2 +-
>  8 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index baf8b1e0f43c..a08f22b53592 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -1510,7 +1510,7 @@ static void rtw_lps_change_dtim_hdl(struct adapter 
> *padapter, u8 dtim)
>   if (pwrpriv->dtim != dtim)
>   pwrpriv->dtim = dtim;
>
> - if ((pwrpriv->bFwCurrentInPSMode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
> + if ((pwrpriv->b_fw_current_in_ps_mode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
>   u8 ps_mode = pwrpriv->pwr_mode;
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index a7e40aaae2d9..51cea6cf46e7 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -1684,7 +1684,7 @@ void rtw_dynamic_check_timer_handler(struct adapter 
> *adapter)
>   if (adapter->net_closed)
>   return;
>
> - if ((adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
> + if ((adapter_to_pwrctl(adapter)->b_fw_current_in_ps_mode)
>   && !(hal_btcoex_IsBtControlLps(adapter))
>   ) {
>   u8 bEnterPS;
> diff --git a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c 
> b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> index f7465cf90c46..21e7a847866f 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> @@ -365,7 +365,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 
> ps_mode, u8 smart_ps, u8 bcn_a
>   rtw_set_rpwm(padapter, PS_STATE_S4);
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> - pwrpriv->bFwCurrentInPSMode = false;
> + pwrpriv->b_fw_current_in_ps_mode = false;
>
>   hal_btcoex_LpsNotify(padapter, ps_mode);
>   }
> @@ -377,7 +377,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 
> ps_mode, u8 smart_ps, u8 bcn_a
>
>   hal_btcoex_LpsNotify(padapter, ps_mode);
>
> - pwrpriv->bFwCurrentInPSMode = true;
> + pwrpriv->b_fw_current_in_ps_mode = true;
>   pwrpriv->pwr_mode = ps_mode;
>   pwrpriv->smart_ps = smart_ps;
>   pwrpriv->bcn_ant_mode = bcn_ant_mode;
> @@ -734,7 +734,7 @@ s32 rtw_register_task_alive(struct adapter *padapter, u32 
> task)
>
>   register_task_alive(pwrctrl, task);
>
> - if (pwrctrl->bFwCurrentInPSMode) {
> + if (pwrctrl->b_fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm < pslv) {
>   if (pwrctrl->cpwm < PS_STATE_S2)
>   res = _FAIL;
> @@ -782,7 +782,7 @@ void rtw_unregister_task_alive(struct adapter *padapter, 
> u32 task)
>
>   unregister_task_alive(pwrctrl, task);
>
> - if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && 
> pwrctrl->bFwCurrentInPSMode) {
> + if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && 
> pwrctrl->b_fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm > pslv)
>   if ((pslv >= PS_STATE_S2) || (pwrctrl->alives == 0))
>   

Re: [Outreachy kernel] [PATCH v5] staging: rtl8723bs: Remove camelcase in several files

[Julia Lawall]

On Fri, 9 Apr 2021, Fabio M. De Francesco wrote:

> Remove camelcase in bFwCurrentInPSMode, a variable used by code
> of several subdirectories/files of the driver. Issue detected by
> checkpatch.pl. Delete the unnecessary "b" (that stands for "byte") from
> the beginning of the name.

Isn't this still against your previous patch?  I see b_ on the removed
lines.

julia

>
> Signed-off-by: Fabio M. De Francesco 
> ---
>
> Changes from v4: Mention the removal of the initial "b" in log message.
> Changes from v3: Fix errors in the format of the patch.
> Changes from v2: Remove unnecessary comment. Shortened a function name.
> Changes from v1: No changes to the code but only to the subject for the
> purpose to differentiate this patch because other removes of camelcase
> have been made in other files of the same directory.
>
>  drivers/staging/rtl8723bs/core/rtw_cmd.c   |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_mlme.c  |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_pwrctrl.c   | 18 +-
>  drivers/staging/rtl8723bs/hal/hal_intf.c   |  2 +-
>  drivers/staging/rtl8723bs/hal/rtl8723b_dm.c|  6 +++---
>  .../staging/rtl8723bs/hal/rtl8723b_hal_init.c  |  2 +-
>  drivers/staging/rtl8723bs/hal/sdio_ops.c   | 14 +++---
>  .../staging/rtl8723bs/include/rtw_pwrctrl.h|  2 +-
>  8 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index a08f22b53592..feb53b8c0ff2 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -1510,7 +1510,7 @@ static void rtw_lps_change_dtim_hdl(struct adapter 
> *padapter, u8 dtim)
>   if (pwrpriv->dtim != dtim)
>   pwrpriv->dtim = dtim;
>
> - if ((pwrpriv->b_fw_current_in_ps_mode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
> + if ((pwrpriv->fw_current_in_ps_mode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
>   u8 ps_mode = pwrpriv->pwr_mode;
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 51cea6cf46e7..895997868c81 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -1684,7 +1684,7 @@ void rtw_dynamic_check_timer_handler(struct adapter 
> *adapter)
>   if (adapter->net_closed)
>   return;
>
> - if ((adapter_to_pwrctl(adapter)->b_fw_current_in_ps_mode)
> + if ((adapter_to_pwrctl(adapter)->fw_current_in_ps_mode)
>   && !(hal_btcoex_IsBtControlLps(adapter))
>   ) {
>   u8 bEnterPS;
> diff --git a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c 
> b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> index 21e7a847866f..481e2ad60853 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> @@ -365,7 +365,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 
> ps_mode, u8 smart_ps, u8 bcn_a
>   rtw_set_rpwm(padapter, PS_STATE_S4);
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> - pwrpriv->b_fw_current_in_ps_mode = false;
> + pwrpriv->fw_current_in_ps_mode = false;
>
>   hal_btcoex_LpsNotify(padapter, ps_mode);
>   }
> @@ -377,7 +377,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 
> ps_mode, u8 smart_ps, u8 bcn_a
>
>   hal_btcoex_LpsNotify(padapter, ps_mode);
>
> - pwrpriv->b_fw_current_in_ps_mode = true;
> + pwrpriv->fw_current_in_ps_mode = true;
>   pwrpriv->pwr_mode = ps_mode;
>   pwrpriv->smart_ps = smart_ps;
>   pwrpriv->bcn_ant_mode = bcn_ant_mode;
> @@ -734,7 +734,7 @@ s32 rtw_register_task_alive(struct adapter *padapter, u32 
> task)
>
>   register_task_alive(pwrctrl, task);
>
> - if (pwrctrl->b_fw_current_in_ps_mode) {
> + if (pwrctrl->fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm < pslv) {
>   if (pwrctrl->cpwm < PS_STATE_S2)
>   res = _FAIL;
> @@ -782,7 +782,7 @@ void rtw_unregister_task_alive(struct adapter *padapter, 
> u32 task)
>
>   unregister_task_alive(pwrctrl, task);
>
> - if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && 
> pwrctrl->b_fw_current_in_ps_mode) {
> + if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && 
> pwrctrl->fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm > pslv)
>   if ((pslv >= PS_STATE_S2) || (pwrctrl->alives == 0))
>   rtw_set_rpwm(padapter, pslv);
> @@ -819,7 +819,7 @@ s32 rtw_register_tx_alive(struct adapter *padapter)
>
>   register_task_alive(pwrctrl, XMIT_ALIVE);
>
> - if 

Re: [Outreachy kernel] [PATCH 2/2] media: zoran: replace bit shifts by BIT() macro

[Julia Lawall]

On Fri, 9 Apr 2021, Mitali Borkar wrote:

> On Fri, Apr 09, 2021 at 12:10:06AM +0200, Julia Lawall wrote:
> >
> >
> > On Fri, 9 Apr 2021, Mitali Borkar wrote:
> >
> > > On Thu, Apr 08, 2021 at 11:15:07PM +0200, Julia Lawall wrote:
> > > >
> > > >
> > > > On Fri, 9 Apr 2021, Mitali Borkar wrote:
> > > >
> > > > > Added #include  and replaced bit shifts by BIT() 
> > > > > macro.
> > > > > This BIT() macro from linux/bitops.h is used to define 
> > > > > ZR36057_VFESPFR_* bitmasks.
> > > > > Use of macro is better and neater. It maintains consistency.
> > > > > Reported by checkpatch.
> > > > >
> > > > > Signed-off-by: Mitali Borkar 
> > > > > ---
> > > > >  drivers/staging/media/zoran/zr36057.h | 10 ++
> > > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/media/zoran/zr36057.h 
> > > > > b/drivers/staging/media/zoran/zr36057.h
> > > > > index a2a75fd9f535..93075459f910 100644
> > > > > --- a/drivers/staging/media/zoran/zr36057.h
> > > > > +++ b/drivers/staging/media/zoran/zr36057.h
> > > > > @@ -8,6 +8,8 @@
> > > > >  #ifndef _ZR36057_H_
> > > > >  #define _ZR36057_H_
> > > > >
> > > > > +#include 
> > > > > +
> > > > >  /* Zoran ZR36057 registers */
> > > > >
> > > > >  #define ZR36057_VFEHCR  0x000/* Video Front End, 
> > > > > Horizontal Configuration Register */
> > > > > @@ -31,12 +33,12 @@
> > > > >  #define ZR36057_VFESPFR_VER_DCM  8
> > > > >  #define ZR36057_VFESPFR_DISP_MODE6
> > > > >  #define ZR36057_VFESPFR_YUV422  (0 << 3)
> > > > > -#define ZR36057_VFESPFR_RGB888  (1 << 3)
> > > > > +#define ZR36057_VFESPFR_RGB888  BIT(3)
> > > >
> > > > Uniformity is generally considered to be more important than using BIT.
> > > > Having only a few constants defined using BIT is a bit strange.
> > > >
> > > Okay Ma'am. Can you please tell me on how to proceed now? I am not sure
> > > how to proceed.
> >
> > I think that this code should just be left as is.
> >
> > Checkpatch makes suggestions.  Its suggestions are not always appropriate
> > for the context.
> >
> Okay Ma'am. This means I should not do any changes now. Do I need to do
> something in this patch now? Or move onto next one?

You can move on.

julia

>
> > julia
> >
> > >
> > > > julia
> > > >
> > > > >  #define ZR36057_VFESPFR_RGB565  (2 << 3)
> > > > >  #define ZR36057_VFESPFR_RGB555  (3 << 3)
> > > > > -#define ZR36057_VFESPFR_ERR_DIF  (1 << 2)
> > > > > -#define ZR36057_VFESPFR_PACK24  (1 << 1)
> > > > > -#define ZR36057_VFESPFR_LITTLE_ENDIAN(1 << 0)
> > > > > +#define ZR36057_VFESPFR_ERR_DIF  BIT(2)
> > > > > +#define ZR36057_VFESPFR_PACK24  BIT(1)
> > > > > +#define ZR36057_VFESPFR_LITTLE_ENDIANBIT(0)
> > > > >
> > > > >  #define ZR36057_VDTR0x00c/* Video Display "Top" 
> > > > > Register */
> > > > >
> > > > > --
> > > > > 2.30.2
> > > > >
> > > > > --
> > > > > 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+unsubscr...@googlegroups.com.
> > > > > To view this discussion on the web visit 
> > > > > https://groups.google.com/d/msgid/outreachy-kernel/ac8ec2b70ac2cc7c541c05a1d9a8db1fe79df793.1617912177.git.mitaliborkar810%40gmail.com.
> > > > >
> > >
>

Re: [Outreachy kernel] [PATCH 2/2] media: zoran: replace bit shifts by BIT() macro

[Julia Lawall]

On Fri, 9 Apr 2021, Mitali Borkar wrote:

> On Thu, Apr 08, 2021 at 11:15:07PM +0200, Julia Lawall wrote:
> >
> >
> > On Fri, 9 Apr 2021, Mitali Borkar wrote:
> >
> > > Added #include  and replaced bit shifts by BIT() macro.
> > > This BIT() macro from linux/bitops.h is used to define ZR36057_VFESPFR_* 
> > > bitmasks.
> > > Use of macro is better and neater. It maintains consistency.
> > > Reported by checkpatch.
> > >
> > > Signed-off-by: Mitali Borkar 
> > > ---
> > >  drivers/staging/media/zoran/zr36057.h | 10 ++
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/zoran/zr36057.h 
> > > b/drivers/staging/media/zoran/zr36057.h
> > > index a2a75fd9f535..93075459f910 100644
> > > --- a/drivers/staging/media/zoran/zr36057.h
> > > +++ b/drivers/staging/media/zoran/zr36057.h
> > > @@ -8,6 +8,8 @@
> > >  #ifndef _ZR36057_H_
> > >  #define _ZR36057_H_
> > >
> > > +#include 
> > > +
> > >  /* Zoran ZR36057 registers */
> > >
> > >  #define ZR36057_VFEHCR  0x000/* Video Front End, Horizontal 
> > > Configuration Register */
> > > @@ -31,12 +33,12 @@
> > >  #define ZR36057_VFESPFR_VER_DCM  8
> > >  #define ZR36057_VFESPFR_DISP_MODE6
> > >  #define ZR36057_VFESPFR_YUV422  (0 << 3)
> > > -#define ZR36057_VFESPFR_RGB888  (1 << 3)
> > > +#define ZR36057_VFESPFR_RGB888  BIT(3)
> >
> > Uniformity is generally considered to be more important than using BIT.
> > Having only a few constants defined using BIT is a bit strange.
> >
> Okay Ma'am. Can you please tell me on how to proceed now? I am not sure
> how to proceed.

I think that this code should just be left as is.

Checkpatch makes suggestions.  Its suggestions are not always appropriate
for the context.

julia

>
> > julia
> >
> > >  #define ZR36057_VFESPFR_RGB565  (2 << 3)
> > >  #define ZR36057_VFESPFR_RGB555  (3 << 3)
> > > -#define ZR36057_VFESPFR_ERR_DIF  (1 << 2)
> > > -#define ZR36057_VFESPFR_PACK24  (1 << 1)
> > > -#define ZR36057_VFESPFR_LITTLE_ENDIAN(1 << 0)
> > > +#define ZR36057_VFESPFR_ERR_DIF  BIT(2)
> > > +#define ZR36057_VFESPFR_PACK24  BIT(1)
> > > +#define ZR36057_VFESPFR_LITTLE_ENDIANBIT(0)
> > >
> > >  #define ZR36057_VDTR0x00c/* Video Display "Top" Register 
> > > */
> > >
> > > --
> > > 2.30.2
> > >
> > > --
> > > 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+unsubscr...@googlegroups.com.
> > > To view this discussion on the web visit 
> > > https://groups.google.com/d/msgid/outreachy-kernel/ac8ec2b70ac2cc7c541c05a1d9a8db1fe79df793.1617912177.git.mitaliborkar810%40gmail.com.
> > >
>

Re: [Outreachy kernel] [PATCH 1/2] media: zoran: add spaces around '<<'

[Julia Lawall]

On Fri, 9 Apr 2021, Mitali Borkar wrote:

> Added spaces around '<<' operator to improve readability and meet linux
> kernel coding style.
> Reported by checkpatch
>
> Signed-off-by: Mitali Borkar 
> ---
>  drivers/staging/media/zoran/zr36057.h | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/media/zoran/zr36057.h 
> b/drivers/staging/media/zoran/zr36057.h
> index 71b651add35a..a2a75fd9f535 100644
> --- a/drivers/staging/media/zoran/zr36057.h
> +++ b/drivers/staging/media/zoran/zr36057.h
> @@ -30,13 +30,13 @@
>  #define ZR36057_VFESPFR_HOR_DCM  14
>  #define ZR36057_VFESPFR_VER_DCM  8
>  #define ZR36057_VFESPFR_DISP_MODE6
> -#define ZR36057_VFESPFR_YUV422  (0<<3)
> -#define ZR36057_VFESPFR_RGB888  (1<<3)
> -#define ZR36057_VFESPFR_RGB565  (2<<3)
> -#define ZR36057_VFESPFR_RGB555  (3<<3)
> -#define ZR36057_VFESPFR_ERR_DIF  (1<<2)
> -#define ZR36057_VFESPFR_PACK24  (1<<1)
> -#define ZR36057_VFESPFR_LITTLE_ENDIAN(1<<0)
> +#define ZR36057_VFESPFR_YUV422  (0 << 3)
> +#define ZR36057_VFESPFR_RGB888  (1 << 3)
> +#define ZR36057_VFESPFR_RGB565  (2 << 3)
> +#define ZR36057_VFESPFR_RGB555  (3 << 3)
> +#define ZR36057_VFESPFR_ERR_DIF  (1 << 2)
> +#define ZR36057_VFESPFR_PACK24  (1 << 1)
> +#define ZR36057_VFESPFR_LITTLE_ENDIAN(1 << 0)

Are these all aligned in the actual file?

julia

>  #define ZR36057_VDTR0x00c/* Video Display "Top" Register 
> */
>
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/8e8ac690d97478f7cbb9b91d46ef7a95ee5f.1617912177.git.mitaliborkar810%40gmail.com.
>

Re: [Outreachy kernel] [PATCH 2/2] media: zoran: replace bit shifts by BIT() macro

[Julia Lawall]

On Fri, 9 Apr 2021, Mitali Borkar wrote:

> Added #include  and replaced bit shifts by BIT() macro.
> This BIT() macro from linux/bitops.h is used to define ZR36057_VFESPFR_* 
> bitmasks.
> Use of macro is better and neater. It maintains consistency.
> Reported by checkpatch.
>
> Signed-off-by: Mitali Borkar 
> ---
>  drivers/staging/media/zoran/zr36057.h | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/zoran/zr36057.h 
> b/drivers/staging/media/zoran/zr36057.h
> index a2a75fd9f535..93075459f910 100644
> --- a/drivers/staging/media/zoran/zr36057.h
> +++ b/drivers/staging/media/zoran/zr36057.h
> @@ -8,6 +8,8 @@
>  #ifndef _ZR36057_H_
>  #define _ZR36057_H_
>
> +#include 
> +
>  /* Zoran ZR36057 registers */
>
>  #define ZR36057_VFEHCR  0x000/* Video Front End, Horizontal 
> Configuration Register */
> @@ -31,12 +33,12 @@
>  #define ZR36057_VFESPFR_VER_DCM  8
>  #define ZR36057_VFESPFR_DISP_MODE6
>  #define ZR36057_VFESPFR_YUV422  (0 << 3)
> -#define ZR36057_VFESPFR_RGB888  (1 << 3)
> +#define ZR36057_VFESPFR_RGB888  BIT(3)

Uniformity is generally considered to be more important than using BIT.
Having only a few constants defined using BIT is a bit strange.

julia

>  #define ZR36057_VFESPFR_RGB565  (2 << 3)
>  #define ZR36057_VFESPFR_RGB555  (3 << 3)
> -#define ZR36057_VFESPFR_ERR_DIF  (1 << 2)
> -#define ZR36057_VFESPFR_PACK24  (1 << 1)
> -#define ZR36057_VFESPFR_LITTLE_ENDIAN(1 << 0)
> +#define ZR36057_VFESPFR_ERR_DIF  BIT(2)
> +#define ZR36057_VFESPFR_PACK24  BIT(1)
> +#define ZR36057_VFESPFR_LITTLE_ENDIANBIT(0)
>
>  #define ZR36057_VDTR0x00c/* Video Display "Top" Register 
> */
>
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/ac8ec2b70ac2cc7c541c05a1d9a8db1fe79df793.1617912177.git.mitaliborkar810%40gmail.com.
>

Re: [Outreachy kernel] [PATCH v3] staging: rtl8723bs: Remove camelcase in several files

[Julia Lawall]

On Thu, 8 Apr 2021, Fabio M. De Francesco wrote:

> Remove camelcase in bFwCurrentInPSMode, a variable used by code
> of several subdirectories/files of the driver. Issue detected by
> checkpatch.pl.

It could be reasonable to mention the removal of b in the log message.

julia

>
> Signed-off-by: Fabio M. De Francesco 
> ---
>
> Changes from v2: Discard v2 because it is a diff against v1 instead of a
> replacement for v1.
> Changes from v1: Rewrite comment for the purpose of specifying which
> variable changes. Shorten its name by removing two unnecessary
> characters (b_).
>
>  drivers/staging/rtl8723bs/core/rtw_cmd.c   |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_mlme.c  |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_pwrctrl.c   | 18 +-
>  drivers/staging/rtl8723bs/hal/hal_intf.c   |  2 +-
>  drivers/staging/rtl8723bs/hal/rtl8723b_dm.c|  6 +++---
>  .../staging/rtl8723bs/hal/rtl8723b_hal_init.c  |  2 +-
>  drivers/staging/rtl8723bs/hal/sdio_ops.c   | 14 +++---
>  .../staging/rtl8723bs/include/rtw_pwrctrl.h|  2 +-
>  8 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index baf8b1e0f43c..a08f22b53592 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -1510,7 +1510,7 @@ static void rtw_lps_change_dtim_hdl(struct adapter 
> *padapter, u8 dtim)
>   if (pwrpriv->dtim != dtim)
>   pwrpriv->dtim = dtim;
>
> - if ((pwrpriv->bFwCurrentInPSMode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
> + if ((pwrpriv->b_fw_current_in_ps_mode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
>   u8 ps_mode = pwrpriv->pwr_mode;
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index a7e40aaae2d9..51cea6cf46e7 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -1684,7 +1684,7 @@ void rtw_dynamic_check_timer_handler(struct adapter 
> *adapter)
>   if (adapter->net_closed)
>   return;
>
> - if ((adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
> + if ((adapter_to_pwrctl(adapter)->b_fw_current_in_ps_mode)
>   && !(hal_btcoex_IsBtControlLps(adapter))
>   ) {
>   u8 bEnterPS;
> diff --git a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c 
> b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> index f7465cf90c46..21e7a847866f 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> @@ -365,7 +365,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 
> ps_mode, u8 smart_ps, u8 bcn_a
>   rtw_set_rpwm(padapter, PS_STATE_S4);
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> - pwrpriv->bFwCurrentInPSMode = false;
> + pwrpriv->b_fw_current_in_ps_mode = false;
>
>   hal_btcoex_LpsNotify(padapter, ps_mode);
>   }
> @@ -377,7 +377,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 
> ps_mode, u8 smart_ps, u8 bcn_a
>
>   hal_btcoex_LpsNotify(padapter, ps_mode);
>
> - pwrpriv->bFwCurrentInPSMode = true;
> + pwrpriv->b_fw_current_in_ps_mode = true;
>   pwrpriv->pwr_mode = ps_mode;
>   pwrpriv->smart_ps = smart_ps;
>   pwrpriv->bcn_ant_mode = bcn_ant_mode;
> @@ -734,7 +734,7 @@ s32 rtw_register_task_alive(struct adapter *padapter, u32 
> task)
>
>   register_task_alive(pwrctrl, task);
>
> - if (pwrctrl->bFwCurrentInPSMode) {
> + if (pwrctrl->b_fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm < pslv) {
>   if (pwrctrl->cpwm < PS_STATE_S2)
>   res = _FAIL;
> @@ -782,7 +782,7 @@ void rtw_unregister_task_alive(struct adapter *padapter, 
> u32 task)
>
>   unregister_task_alive(pwrctrl, task);
>
> - if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && 
> pwrctrl->bFwCurrentInPSMode) {
> + if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && 
> pwrctrl->b_fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm > pslv)
>   if ((pslv >= PS_STATE_S2) || (pwrctrl->alives == 0))
>   rtw_set_rpwm(padapter, pslv);
> @@ -819,7 +819,7 @@ s32 rtw_register_tx_alive(struct adapter *padapter)
>
>   register_task_alive(pwrctrl, XMIT_ALIVE);
>
> - if (pwrctrl->bFwCurrentInPSMode) {
> + if (pwrctrl->b_fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm < pslv) {
>   if (pwrctrl->cpwm < PS_STATE_S2)
>   res = _FAIL;
> @@ -864,7 +864,7 @@ s32 

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove camelcase in several files

[Julia Lawall]

On Thu, 8 Apr 2021, Fabio M. De Francesco wrote:

> On Thursday, April 8, 2021 10:36:32 AM CEST Greg KH wrote:
> > On Thu, Apr 08, 2021 at 10:07:14AM +0200, Fabio M. De Francesco wrote:
> > > Remove camelcase in a symbol that is used by several files.
> >
> > What symbol?
> >
>
> I'm not sure I understand what you are asking. You mean
> that I have to specify the name of the variable that I changed?
> Or you mean that I shouldn't use the term "symbol" but write
> "variable", "function", "macro", and so on?

He wants the name of the symbol.  Each changed line includes lots of
symbols, so it is a significant effort to scan the patch to see what
symbol is being changed.

julia

>
> >
> > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > >   u8 LpsIdleCount;
> > >   u8 power_mgnt;
> > >   u8 org_power_mgnt;
> > > - u8 bFwCurrentInPSMode;
> > > + u8 b_fw_current_in_ps_mode;
> >
> > The "b" here means "byte" so you can drop the "b_" as that means
> > nothing, we do not use this type of notation in the kernel as the
> > compiler can check it for us.
> >
>
> OK, I didn't know what the 'b' meant in this context. I'll drop it.
>
> Thanks,
>
> Fabio
>
> > thanks,
> >
> > greg k-h
> >
>
>
>
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/2284292.R3b5UFg5HO%40localhost.localdomain.
>

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove camelcase in several files

[Julia Lawall]

On Thu, 8 Apr 2021, Fabio M. De Francesco wrote:

> Remove camelcase in a symbol that is used by several files.

In addition to Greg's suggestion about dropping the b, there are some more
things you can do to improve the usage of this field.

First, I noticed that the type is u8.  It can be changed to bool.

Second, the == true doesn't seem necessary in the following:

if (adapter_to_pwrctl(padapter)->bFwCurrentInPSMode == true) {

Of course before doing any of these, it would be good to check all of the
uses and be sure that the only possible values in this field are true and
false.

julia

>
> Signed-off-by: Fabio M. De Francesco 
> ---
>  drivers/staging/rtl8723bs/core/rtw_cmd.c   |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_mlme.c  |  2 +-
>  drivers/staging/rtl8723bs/core/rtw_pwrctrl.c   | 18 +-
>  drivers/staging/rtl8723bs/hal/hal_intf.c   |  2 +-
>  drivers/staging/rtl8723bs/hal/rtl8723b_dm.c|  6 +++---
>  .../staging/rtl8723bs/hal/rtl8723b_hal_init.c  |  2 +-
>  drivers/staging/rtl8723bs/hal/sdio_ops.c   | 14 +++---
>  .../staging/rtl8723bs/include/rtw_pwrctrl.h|  2 +-
>  8 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index baf8b1e0f43c..a08f22b53592 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -1510,7 +1510,7 @@ static void rtw_lps_change_dtim_hdl(struct adapter 
> *padapter, u8 dtim)
>   if (pwrpriv->dtim != dtim)
>   pwrpriv->dtim = dtim;
>
> - if ((pwrpriv->bFwCurrentInPSMode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
> + if ((pwrpriv->b_fw_current_in_ps_mode == true) && (pwrpriv->pwr_mode > 
> PS_MODE_ACTIVE)) {
>   u8 ps_mode = pwrpriv->pwr_mode;
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index a7e40aaae2d9..51cea6cf46e7 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -1684,7 +1684,7 @@ void rtw_dynamic_check_timer_handler(struct adapter 
> *adapter)
>   if (adapter->net_closed)
>   return;
>
> - if ((adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
> + if ((adapter_to_pwrctl(adapter)->b_fw_current_in_ps_mode)
>   && !(hal_btcoex_IsBtControlLps(adapter))
>   ) {
>   u8 bEnterPS;
> diff --git a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c 
> b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> index f7465cf90c46..21e7a847866f 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> @@ -365,7 +365,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 
> ps_mode, u8 smart_ps, u8 bcn_a
>   rtw_set_rpwm(padapter, PS_STATE_S4);
>
>   rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 
> *)(_mode));
> - pwrpriv->bFwCurrentInPSMode = false;
> + pwrpriv->b_fw_current_in_ps_mode = false;
>
>   hal_btcoex_LpsNotify(padapter, ps_mode);
>   }
> @@ -377,7 +377,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 
> ps_mode, u8 smart_ps, u8 bcn_a
>
>   hal_btcoex_LpsNotify(padapter, ps_mode);
>
> - pwrpriv->bFwCurrentInPSMode = true;
> + pwrpriv->b_fw_current_in_ps_mode = true;
>   pwrpriv->pwr_mode = ps_mode;
>   pwrpriv->smart_ps = smart_ps;
>   pwrpriv->bcn_ant_mode = bcn_ant_mode;
> @@ -734,7 +734,7 @@ s32 rtw_register_task_alive(struct adapter *padapter, u32 
> task)
>
>   register_task_alive(pwrctrl, task);
>
> - if (pwrctrl->bFwCurrentInPSMode) {
> + if (pwrctrl->b_fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm < pslv) {
>   if (pwrctrl->cpwm < PS_STATE_S2)
>   res = _FAIL;
> @@ -782,7 +782,7 @@ void rtw_unregister_task_alive(struct adapter *padapter, 
> u32 task)
>
>   unregister_task_alive(pwrctrl, task);
>
> - if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && 
> pwrctrl->bFwCurrentInPSMode) {
> + if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && 
> pwrctrl->b_fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm > pslv)
>   if ((pslv >= PS_STATE_S2) || (pwrctrl->alives == 0))
>   rtw_set_rpwm(padapter, pslv);
> @@ -819,7 +819,7 @@ s32 rtw_register_tx_alive(struct adapter *padapter)
>
>   register_task_alive(pwrctrl, XMIT_ALIVE);
>
> - if (pwrctrl->bFwCurrentInPSMode) {
> + if (pwrctrl->b_fw_current_in_ps_mode) {
>   if (pwrctrl->cpwm < pslv) {
>   if (pwrctrl->cpwm < PS_STATE_S2)
>   

Re: [Outreachy kernel] [PATCH] staging: rtl8712: added spaces around '+'

[Julia Lawall]

The subject line should be in the imperative, so "add" instead of "added".

On Thu, 8 Apr 2021, Mitali Borkar wrote:

> Clean up Check:spaces preferred around that '+' (ctx:VxV)
> Reported by checkpatch

Please try to rephrase to explain what you did and why.  "Clean up" kind
of states what the goal is, but your opinion about what is a clean up
might be different than someone else's.  It's also not necessary to cite
the checkpatch warning exactly.

julia

>
> Signed-off-by: Mitali Borkar 
> ---
>  drivers/staging/rtl8712/wlan_bssdef.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8712/wlan_bssdef.h 
> b/drivers/staging/rtl8712/wlan_bssdef.h
> index b54ccaacc527..ec3749813728 100644
> --- a/drivers/staging/rtl8712/wlan_bssdef.h
> +++ b/drivers/staging/rtl8712/wlan_bssdef.h
> @@ -176,7 +176,7 @@ struct NDIS_802_11_WEP {
>  #define MIC_CHECK_TIME   6000
>
>  #ifndef Ndis802_11APMode
> -#define Ndis802_11APMode (Ndis802_11InfrastructureMax+1)
> +#define Ndis802_11APMode (Ndis802_11InfrastructureMax + 1)
>  #endif
>
>  struct   wlan_network {
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/YG690ZIRdCEcjoM6%40kali.
>

[PATCH] clk: fix semicolon.cocci warnings

[Julia Lawall]

From: kernel test robot 

Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

CC: Liam Beguin 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

url:
https://github.com/0day-ci/linux/commits/Liam-Beguin/add-support-for-the-lmk04832/20210407-085408
base:   f40ddce88593482919761f74910f42f4b84c004b
:: branch date: 9 hours ago
:: commit date: 9 hours ago

 clk-lmk04832.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/clk/clk-lmk04832.c
+++ b/drivers/clk/clk-lmk04832.c
@@ -1172,7 +1172,7 @@ static int lmk04832_probe(struct spi_dev

lmk->clkout[reg].sysref =
of_property_read_bool(child, "ti,clkout-sysref");
-   };
+   }

lmk->regmap = devm_regmap_init_spi(spi, _config);
if (IS_ERR(lmk->regmap)) {

[PATCH] clk: fix for_each_child.cocci warnings

[Julia Lawall]

From: kernel test robot 

For_each_child_of_node should have of_node_put() before goto.

Generated by: scripts/coccinelle/iterators/for_each_child.cocci

CC: Liam Beguin 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

url:
https://github.com/0day-ci/linux/commits/Liam-Beguin/add-support-for-the-lmk04832/20210407-085408
base:   f40ddce88593482919761f74910f42f4b84c004b
:: branch date: 9 hours ago
:: commit date: 9 hours ago

 clk-lmk04832.c |1 +
 1 file changed, 1 insertion(+)

--- a/drivers/clk/clk-lmk04832.c
+++ b/drivers/clk/clk-lmk04832.c
@@ -1159,6 +1159,7 @@ static int lmk04832_probe(struct spi_dev
if (ret) {
dev_err(lmk->dev, "missing reg property in child: %s\n",
child->full_name);
+   of_node_put(child);
goto err_disable_oscin;
}

Re: [PATCH] inotify: fix minmax.cocci warnings

[Julia Lawall]

On Wed, 7 Apr 2021, Jan Kara wrote:

> On Tue 06-04-21 22:49:26, Julia Lawall wrote:
> > From: kernel test robot 
> >
> > Opportunity for min().
> >
> > Generated by: scripts/coccinelle/misc/minmax.cocci
> >
> > Fixes: 8636e3295ce3 ("coccinelle: misc: add minmax script")
> > CC: Denis Efremov 
> > Reported-by: kernel test robot 
> > Signed-off-by: kernel test robot 
> > Signed-off-by: Julia Lawall 
> ...
> > --- a/fs/notify/inotify/inotify_user.c
> > +++ b/fs/notify/inotify/inotify_user.c
> > @@ -382,7 +382,7 @@ static int inotify_add_to_idr(struct idr
> >
> > spin_unlock(idr_lock);
> > idr_preload_end();
> > -   return ret < 0 ? ret : 0;
> > +   return min(ret, 0);
> >  }
>
> Honestly, while previous expression is a standard idiom for "if 'ret' holds
> an error, return it", the new expression is harder to understand for me. So
> I prefer to keep things as they are in this particular case...

OK, I had doubts about it as well, but I forwarded it because I found them
equally obscure...

Denis, maybe the semantic patch should be updated to avoid this case.

julia

Re: [Outreachy kernel] [PATCH 1/2] staging: media: omap4iss: Ending line with argument

[Julia Lawall]

On Wed, 7 Apr 2021, Beatriz Martins de Carvalho wrote:

>
> Em 01/04/21 16:28, Matthew Wilcox escreveu:
> > On Thu, Apr 01, 2021 at 04:07:38PM +0100, Beatriz Martins de Carvalho wrote:
> > > diff --git a/drivers/staging/media/omap4iss/iss.c
> > > b/drivers/staging/media/omap4iss/iss.c
> > > index dae9073e7d3c..e8f724dbf810 100644
> > > --- a/drivers/staging/media/omap4iss/iss.c
> > > +++ b/drivers/staging/media/omap4iss/iss.c
> > > @@ -559,9 +559,10 @@ static int iss_reset(struct iss_device *iss)
> > >   iss_reg_set(iss, OMAP4_ISS_MEM_TOP, ISS_HL_SYSCONFIG,
> > >   ISS_HL_SYSCONFIG_SOFTRESET);
> > >   -   timeout = iss_poll_condition_timeout(
> > > - !(iss_reg_read(iss, OMAP4_ISS_MEM_TOP, ISS_HL_SYSCONFIG) &
> > > - ISS_HL_SYSCONFIG_SOFTRESET), 1000, 10, 100);
> > > + timeout = iss_poll_condition_timeout(!(iss_reg_read(iss,
> > > + OMAP4_ISS_MEM_TOP,
> > > ISS_HL_SYSCONFIG)
> > > + &
> > > ISS_HL_SYSCONFIG_SOFTRESET),
> > > + 1000, 10, 100);
> > This is not a readability improvment.  I would factor it out into its
> > own function.
>
> Thanks for the review. How can I do this? I don't know how to do this.

Copy the code into a new function.  Then see what parameters this function
needs for the various information it requires.  The code will produce some
results that are needed by the rest of the program.  So you have to
arrange via the return value that the proper variables are initialized
after the function call.

For example,

x = a + b;

could become

int f(int a, int b) {
  return a + b;
}

x = f(a,b);

That is a pretty silly change, but it gives the idea.

julia

> Beatriz Martins de Carvalho
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/e175859f-4f68-0408-415f-a3e19f7e4874%40gmail.com.
>

[PATCH] inotify: fix minmax.cocci warnings

[Julia Lawall]

From: kernel test robot 

Opportunity for min().

Generated by: scripts/coccinelle/misc/minmax.cocci

Fixes: 8636e3295ce3 ("coccinelle: misc: add minmax script")
CC: Denis Efremov 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/jlawall/linux.git 
for-5.12
head:   cbc8ed0b4f7eeb782c153ec88d6d20bc0f0ca3a7
commit: 8636e3295ce33515c50ef728f0ff3800d97f9f44 [1/4] coccinelle: misc: add 
minmax script
:: branch date: 2 days ago
:: commit date: 2 weeks ago

 inotify_user.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -382,7 +382,7 @@ static int inotify_add_to_idr(struct idr

spin_unlock(idr_lock);
idr_preload_end();
-   return ret < 0 ? ret : 0;
+   return min(ret, 0);
 }

 static struct inotify_inode_mark *inotify_idr_find_locked(struct 
fsnotify_group *group,

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: hal: Add spaces around operators in HalBtc8723b2Ant.c

[Julia Lawall]

On Tue, 6 Apr 2021, Fabio M. De Francesco wrote:

> Added spaces around operators in file HalBtc8723b2Ant.c. Issue detected
> by checkpatch.pl. Spaces are preferred to improve readibility.

You don't usually need the file name in the subject line or the commit
message.  One can easily see the file from the diffstat below.  The
subject line should be concise, and the log message should focus on what
you have done (briefly) and why.

julia

>
> Signed-off-by: Fabio M. De Francesco 
> ---
>  .../staging/rtl8723bs/hal/HalBtc8723b2Ant.c   | 78 +--
>  1 file changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c 
> b/drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c
> index 6edaefa47af1..4b570ec75e67 100644
> --- a/drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c
> +++ b/drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c
> @@ -44,7 +44,7 @@ static u8 halbtc8723b2ant_BtRssiState(
>   (pCoexSta->preBtRssiState == BTC_RSSI_STATE_LOW) ||
>   (pCoexSta->preBtRssiState == BTC_RSSI_STATE_STAY_LOW)
>   ) {
> - if (btRssi >= 
> (rssiThresh+BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
> + if (btRssi >= (rssiThresh + 
> BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
>   btRssiState = BTC_RSSI_STATE_HIGH;
>   BTC_PRINT(BTC_MSG_ALGORITHM, 
> ALGO_BT_RSSI_STATE, ("[BTCoex], BT Rssi state switch to High\n"));
>   } else {
> @@ -70,7 +70,7 @@ static u8 halbtc8723b2ant_BtRssiState(
>   (pCoexSta->preBtRssiState == BTC_RSSI_STATE_LOW) ||
>   (pCoexSta->preBtRssiState == BTC_RSSI_STATE_STAY_LOW)
>   ) {
> - if (btRssi >= 
> (rssiThresh+BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
> + if (btRssi >= (rssiThresh + 
> BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
>   btRssiState = BTC_RSSI_STATE_MEDIUM;
>   BTC_PRINT(BTC_MSG_ALGORITHM, 
> ALGO_BT_RSSI_STATE, ("[BTCoex], BT Rssi state switch to Medium\n"));
>   } else {
> @@ -81,7 +81,7 @@ static u8 halbtc8723b2ant_BtRssiState(
>   (pCoexSta->preBtRssiState == BTC_RSSI_STATE_MEDIUM) ||
>   (pCoexSta->preBtRssiState == BTC_RSSI_STATE_STAY_MEDIUM)
>   ) {
> - if (btRssi >= 
> (rssiThresh1+BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
> + if (btRssi >= (rssiThresh1 + 
> BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
>   btRssiState = BTC_RSSI_STATE_HIGH;
>   BTC_PRINT(BTC_MSG_ALGORITHM, 
> ALGO_BT_RSSI_STATE, ("[BTCoex], BT Rssi state switch to High\n"));
>   } else if (btRssi < rssiThresh) {
> @@ -125,7 +125,7 @@ static u8 halbtc8723b2ant_WifiRssiState(
>   (pCoexSta->preWifiRssiState[index] == 
> BTC_RSSI_STATE_LOW) ||
>   (pCoexSta->preWifiRssiState[index] == 
> BTC_RSSI_STATE_STAY_LOW)
>   ) {
> - if (wifiRssi >= 
> (rssiThresh+BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
> + if (wifiRssi >= (rssiThresh + 
> BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
>   wifiRssiState = BTC_RSSI_STATE_HIGH;
>   BTC_PRINT(BTC_MSG_ALGORITHM, 
> ALGO_WIFI_RSSI_STATE, ("[BTCoex], wifi RSSI state switch to High\n"));
>   } else {
> @@ -151,7 +151,7 @@ static u8 halbtc8723b2ant_WifiRssiState(
>   (pCoexSta->preWifiRssiState[index] == 
> BTC_RSSI_STATE_LOW) ||
>   (pCoexSta->preWifiRssiState[index] == 
> BTC_RSSI_STATE_STAY_LOW)
>   ) {
> - if (wifiRssi >= 
> (rssiThresh+BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
> + if (wifiRssi >= (rssiThresh + 
> BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
>   wifiRssiState = BTC_RSSI_STATE_MEDIUM;
>   BTC_PRINT(BTC_MSG_ALGORITHM, 
> ALGO_WIFI_RSSI_STATE, ("[BTCoex], wifi RSSI state switch to Medium\n"));
>   } else {
> @@ -162,7 +162,7 @@ static u8 halbtc8723b2ant_WifiRssiState(
>   (pCoexSta->preWifiRssiState[index] == 
> BTC_RSSI_STATE_MEDIUM) ||
>   (pCoexSta->preWifiRssiState[index] == 
> BTC_RSSI_STATE_STAY_MEDIUM)
>   ) {
> - if (wifiRssi >= 
> (rssiThresh1+BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
> + if (wifiRssi >= (rssiThresh1 + 
> BTC_RSSI_COEX_THRESH_TOL_8723B_2ANT)) {
>   wifiRssiState = BTC_RSSI_STATE_HIGH;
>   BTC_PRINT(BTC_MSG_ALGORITHM, 
> ALGO_WIFI_RSSI_STATE, ("[BTCoex], wifi RSSI state switch to High\n"));
>   } else if (wifiRssi < rssiThresh) {
> @@ 

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: hal: Add spaces around operator in HalBtc8723b1Ant.h

[Julia Lawall]

On Tue, 6 Apr 2021, Fabio M. De Francesco wrote:

> Added spaces around operators in file HalBtc8723b1Ant.h. Issue detected
> by checkpatch.pl. Spaces are preferred to improve readibility.
>
> Signed-off-by: Fabio M. De Francesco 
> ---
>  drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.h 
> b/drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.h
> index 59e8c68cdc20..719e19420a3b 100644
> --- a/drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.h
> +++ b/drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.h
> @@ -15,7 +15,7 @@
>  #define  BT_INFO_8723B_1ANT_B_CONNECTION BIT0
>
>  #define  BT_INFO_8723B_1ANT_A2DP_BASIC_RATE(_BT_INFO_EXT_)   \
> - (((_BT_INFO_EXT_)) ? true : false)
> + (((_BT_INFO_EXT_ & BIT0)) ? true : false)

There seem to be a lot of parentheses in this code (( )).  Maybe you can
make a series and fix that problem too.

julia

>
>  #define  BTC_RSSI_COEX_THRESH_TOL_8723B_1ANT 2
>
> --
> 2.30.2
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210406164001.13646-1-fmdefrancesco%40gmail.com.
>

Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: hal: Remove camelcase in Hal8723BReg.h

[Julia Lawall]

On Tue, 6 Apr 2021, FMDF wrote:

> On Tue, Apr 6, 2021 at 4:11 PM Greg KH  wrote:
> >
> > On Tue, Apr 06, 2021 at 03:05:56PM +0200, Fabio M. De Francesco wrote:
> > > Remove camelcase in some symbols defined in Hal8723BReg.h. These symbols
> > > are not used anywhere else, therefore this patch does not break the 
> > > driver.
> > >
> > > Signed-off-by: Fabio M. De Francesco 
> > > ---
> > >  drivers/staging/rtl8723bs/hal/Hal8723BReg.h | 16 
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > If this is "v2", you need to put below the --- line what changed from
> > v1.
>
> It is a v2 only because you made me notice that I forgot to cc
> linux-staging and linux-kernel.
> So I sent it again, but probably I shouldn't have changed the version
> number because nothing else had changed.
>
> > Please fix up and send a v3.
> >
>
> No problem at all. I can explain in the patch v3 what I wrote above.
> May you please confirm that a patch v3 is the correct solution even
> when nothing changes in the code?

When Greg acknowledges your patch, everyone who is concerned with the
patch should see it.  So he needs to pick up the right version of the
patch that has everyone in CC.  He can use the version number to choose
the most recent one.

The version numbers don't appear anywhere once the code is committed.  So
it's not a problem to move up to the next version for any reason.  On the
other hand, if you skip a version, it can be a problem, because someone
may wonder if they missed some useful information.

julia

>
> Thanks for your help,
>
> Fabio
>
> > thanks,
> >
> > greg k-h
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/CAPj211u9MHgtjeZGUPsLxU3HkbJ8cr2EUL0v9HA7LE9-b1uUoA%40mail.gmail.com.
>

Re: [Outreachy kernel] [PATCH 1/2] staging: rtl8712: Rewrite NULL comparisons

[Julia Lawall]

On Tue, 6 Apr 2021, Zhansaya Bagdauletkyzy wrote:

> Replace NULL comparisons with boolean negation.

This summarizes concisely what you did, which is helpful, but you could
also say why.  For example, to make the code more concise or to improve
readability or for consistency with the rest of the code base.

julia

> Reported by checkpatch.
>
> Signed-off-by: Zhansaya Bagdauletkyzy 
> ---
>  drivers/staging/rtl8712/rtl871x_recv.h | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/rtl871x_recv.h 
> b/drivers/staging/rtl8712/rtl871x_recv.h
> index 7c9995060a6f..4aa39f4f3b84 100644
> --- a/drivers/staging/rtl8712/rtl871x_recv.h
> +++ b/drivers/staging/rtl8712/rtl871x_recv.h
> @@ -135,7 +135,7 @@ int recv_func(struct _adapter *padapter, void *pcontext);
>  static inline u8 *get_rxmem(union recv_frame *precvframe)
>  {
>   /* always return rx_head... */
> - if (precvframe == NULL)
> + if (!precvframe)
>   return NULL;
>   return precvframe->u.hdr.rx_head;
>  }
> @@ -143,7 +143,7 @@ static inline u8 *get_rxmem(union recv_frame *precvframe)
>  static inline u8 *get_recvframe_data(union recv_frame *precvframe)
>  {
>   /* always return rx_data */
> - if (precvframe == NULL)
> + if (!precvframe)
>   return NULL;
>   return precvframe->u.hdr.rx_data;
>  }
> @@ -153,7 +153,7 @@ static inline u8 *recvframe_pull(union recv_frame 
> *precvframe, sint sz)
>   /* used for extract sz bytes from rx_data, update rx_data and return
>* the updated rx_data to the caller
>*/
> - if (precvframe == NULL)
> + if (!precvframe)
>   return NULL;
>   precvframe->u.hdr.rx_data += sz;
>   if (precvframe->u.hdr.rx_data > precvframe->u.hdr.rx_tail) {
> @@ -170,7 +170,7 @@ static inline u8 *recvframe_put(union recv_frame 
> *precvframe, sint sz)
>* return the updated rx_tail to the caller
>* after putting, rx_tail must be still larger than rx_end.
>*/
> - if (precvframe == NULL)
> + if (!precvframe)
>   return NULL;
>   precvframe->u.hdr.rx_tail += sz;
>   if (precvframe->u.hdr.rx_tail > precvframe->u.hdr.rx_end) {
> @@ -188,7 +188,7 @@ static inline u8 *recvframe_pull_tail(union recv_frame 
> *precvframe, sint sz)
>* updated rx_end to the caller
>* after pulling, rx_end must be still larger than rx_data.
>*/
> - if (precvframe == NULL)
> + if (!precvframe)
>   return NULL;
>   precvframe->u.hdr.rx_tail -= sz;
>   if (precvframe->u.hdr.rx_tail < precvframe->u.hdr.rx_data) {
> --
> 2.25.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/5d2db1d233030ececcdd4934ca9c46cb998c0c5b.1617705825.git.zhansayabagdaulet%40gmail.com.
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: core: align arguments with open parenthesis

[Julia Lawall]

On Mon, 5 Apr 2021, Beatriz Martins de Carvalho wrote:

> Cleans up checks of "Alignment should match open parenthesis"
> in file rtw_ap.c
>
> Signed-off-by: Beatriz Martins de Carvalho 
> 
> ---
>  drivers/staging/rtl8723bs/core/rtw_ap.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c 
> b/drivers/staging/rtl8723bs/core/rtw_ap.c
> index ca6fec52d213..73a35b3320fe 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ap.c
> @@ -439,7 +439,7 @@ void add_RATid(struct adapter *padapter, struct sta_info 
> *psta, u8 rssi_level)
>   arg[3] = psta->init_rate;
>
>   DBG_871X("%s => mac_id:%d , raid:%d , shortGIrate =%d, bitmap = 
> 0x%x\n",
> - __func__, psta->mac_id, psta->raid, shortGIrate, 
> tx_ra_bitmap);
> +  __func__, psta->mac_id, psta->raid, shortGIrate, 
> tx_ra_bitmap);
>
>   rtw_hal_add_ra_tid(padapter, tx_ra_bitmap, arg, rssi_level);
>   } else {
> @@ -512,7 +512,7 @@ void update_bmc_sta(struct adapter *padapter)
>   arg[3] = psta->init_rate;
>
>   DBG_871X("%s => mac_id:%d , raid:%d , bitmap = 0x%x\n",
> - __func__, psta->mac_id, psta->raid, 
> tx_ra_bitmap);
> +  __func__, psta->mac_id, psta->raid, 
> tx_ra_bitmap);
>
>   rtw_hal_add_ra_tid(padapter, tx_ra_bitmap, arg, 0);
>   }
> @@ -605,14 +605,14 @@ void update_sta_info_apmode(struct adapter *padapter, 
> struct sta_info *psta)
>
>   /*  B0 Config LDPC Coding Capability */
>   if (TEST_FLAG(phtpriv_ap->ldpc_cap, LDPC_HT_ENABLE_TX) &&
> - GET_HT_CAPABILITY_ELE_LDPC_CAP((u8 
> *)(_sta->ht_cap))) {
> +   GET_HT_CAPABILITY_ELE_LDPC_CAP((u8 
> *)(_sta->ht_cap))) {
>   SET_FLAG(cur_ldpc_cap, (LDPC_HT_ENABLE_TX | 
> LDPC_HT_CAP_TX));
>   DBG_871X("Enable HT Tx LDPC for STA(%d)\n", psta->aid);
>   }

TEST_FLAG and SET_FLAG are not standard for the kernel, don't do anything
very interesting, and aren't even used consistently in this file.  Maybe
you could get rid of them completely from the driver.  In another patch,
of course.

julia


>   /*  B7 B8 B9 Config STBC setting */
>   if (TEST_FLAG(phtpriv_ap->stbc_cap, STBC_HT_ENABLE_TX) &&
> - GET_HT_CAPABILITY_ELE_RX_STBC((u8 
> *)(_sta->ht_cap))) {
> +   GET_HT_CAPABILITY_ELE_RX_STBC((u8 
> *)(_sta->ht_cap))) {
>   SET_FLAG(cur_stbc_cap, (STBC_HT_ENABLE_TX | 
> STBC_HT_CAP_TX));
>   DBG_871X("Enable HT Tx STBC for STA(%d)\n", psta->aid);
>   }
> @@ -1176,7 +1176,7 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 
> *pbuf,  int len)
>   );
>
>   if ((psecuritypriv->wpa_pairwise_cipher & WPA_CIPHER_CCMP) ||
> - (psecuritypriv->wpa2_pairwise_cipher & 
> WPA_CIPHER_CCMP)) {
> +  (psecuritypriv->wpa2_pairwise_cipher & WPA_CIPHER_CCMP)) {
>   pht_cap->ampdu_params_info |= 
> (IEEE80211_HT_CAP_AMPDU_DENSITY & (0x07 << 2));
>   } else {
>   pht_cap->ampdu_params_info |= 
> (IEEE80211_HT_CAP_AMPDU_DENSITY & 0x00);
> @@ -1233,7 +1233,7 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 
> *pbuf,  int len)
>   pmlmepriv->htpriv.ht_option = false;
>
>   if ((psecuritypriv->wpa2_pairwise_cipher & WPA_CIPHER_TKIP) ||
> -   (psecuritypriv->wpa_pairwise_cipher & WPA_CIPHER_TKIP)) {
> +  (psecuritypriv->wpa_pairwise_cipher & WPA_CIPHER_TKIP)) {
>   /* todo: */
>   /* ht_cap = false; */
>   }
> @@ -1820,7 +1820,7 @@ static int rtw_ht_operation_update(struct adapter 
> *padapter)
>   }
>
>   DBG_871X("%s new operation mode = 0x%X changes =%d\n",
> -__func__, pmlmepriv->ht_op_mode, op_mode_changes);
> +  __func__, pmlmepriv->ht_op_mode, op_mode_changes);
>
>   return op_mode_changes;
>  }
> @@ -1865,7 +1865,7 @@ void bss_cap_update_on_sta_join(struct adapter 
> *padapter, struct sta_info *psta)
>   pmlmepriv->num_sta_no_short_preamble++;
>
>   if ((pmlmeext->cur_wireless_mode > WIRELESS_11B) &&
> - (pmlmepriv->num_sta_no_short_preamble == 1)) {
> + (pmlmepriv->num_sta_no_short_preamble == 1)) {
>   beacon_updated = true;
>   update_beacon(padapter, 0xFF, NULL, true);
>   }
> --
> 2.25.1
>
> --
> 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 

Re: [Outreachy kernel] [PATCH] staging: emxx_udc: Ending line with argument

[Julia Lawall]

On Mon, 5 Apr 2021, Beatriz Martins de Carvalho wrote:

>
> Em 01/04/21 22:16, Julia Lawall escreveu:
> >
> > On Thu, 1 Apr 2021, Beatriz Martins de Carvalho wrote:
> >
> > > Cleans up check of "Lines should not end with a '('"
> > > with argument present in next line in file emxx_udc.c
> > The coding style documentation encourages remaining within 80 characters.
> > I'm not sure that the ( warning is worth increading the excess beyond 80
> > characters (or introducing it in the last case).
> >
> > julia
> Thanks, prof Julia, how the checkpath now warning only if line length of 100
> characters, I used it for reference.

I think it was changed so that people would leave as is code that was
really more readable at 85 or 90 characters.  But the documentation still
encourages 80 characters.

julia

>
> I will take the suggestions of checkpatch with more attention.
>
> Beatriz
>
> > > Signed-off-by: Beatriz Martins de Carvalho
> > > 
> > > ---
> > >   drivers/staging/emxx_udc/emxx_udc.c | 11 ---
> > >   1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/staging/emxx_udc/emxx_udc.c
> > > b/drivers/staging/emxx_udc/emxx_udc.c
> > > index 741147a4f0fe..20f53cf6e20f 100644
> > > --- a/drivers/staging/emxx_udc/emxx_udc.c
> > > +++ b/drivers/staging/emxx_udc/emxx_udc.c
> > > @@ -1073,9 +1073,8 @@ static int _nbu2ss_epn_in_pio(struct nbu2ss_udc
> > > *udc, struct nbu2ss_ep *ep,
> > >   i_word_length = length / sizeof(u32);
> > >   if (i_word_length > 0) {
> > >   for (i = 0; i < i_word_length; i++) {
> > > - _nbu2ss_writel(
> > > - >EP_REGS[ep->epnum -
> > > 1].EP_WRITE,
> > > - p_buf_32->dw);
> > > + _nbu2ss_writel(>EP_REGS[ep->epnum -
> > > 1].EP_WRITE,
> > > +p_buf_32->dw);
> > >
> > >   p_buf_32++;
> > >   }
> > > @@ -1225,8 +1224,7 @@ static void _nbu2ss_restert_transfer(struct
> > > nbu2ss_ep *ep)
> > >   return;
> > >
> > >   if (ep->epnum > 0) {
> > > - length = _nbu2ss_readl(
> > > - >udc->p_regs->EP_REGS[ep->epnum - 1].EP_LEN_DCNT);
> > > + length = _nbu2ss_readl(>udc->p_regs->EP_REGS[ep->epnum -
> > > 1].EP_LEN_DCNT);
> > >
> > >   length &= EPN_LDATA;
> > >   if (length < ep->ep.maxpacket)
> > > @@ -1462,8 +1460,7 @@ static void _nbu2ss_epn_set_stall(struct nbu2ss_udc
> > > *udc,
> > >   for (limit_cnt = 0
> > >   ; limit_cnt < IN_DATA_EMPTY_COUNT
> > >   ; limit_cnt++) {
> > > - regdata = _nbu2ss_readl(
> > > - >EP_REGS[ep->epnum - 1].EP_STATUS);
> > > + regdata = _nbu2ss_readl(>EP_REGS[ep->epnum -
> > > 1].EP_STATUS);
> > >
> > >   if ((regdata & EPN_IN_DATA) == 0)
> > >   break;
> > > --
> > > 2.25.1
> > >
> > > --
> > > 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+unsubscr...@googlegroups.com.
> > > To view this discussion on the web visit
> > > https://groups.google.com/d/msgid/outreachy-kernel/20210401195457.24512-1-martinsdecarvalhobeatriz%40gmail.com.
> > >
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/82580331-13dc-ed79-72e7-3984fd2f75f9%40gmail.com.
>

Re: [PATCH net-next v8 2/2] net: Add Qcom WWAN control driver (fwd)

[Julia Lawall]

There is a clear use after free on line 213.

julia

-- Forwarded message --
Date: Sat, 3 Apr 2021 04:42:45 +0800
From: kernel test robot 
To: kbu...@lists.01.org
Cc: l...@intel.com, Julia Lawall 
Subject: Re: [PATCH net-next v8 2/2] net: Add Qcom WWAN control driver

CC: kbuild-...@lists.01.org
In-Reply-To: <1617372397-13988-2-git-send-email-loic.poul...@linaro.org>
References: <1617372397-13988-2-git-send-email-loic.poul...@linaro.org>
TO: Loic Poulain 
TO: gre...@linuxfoundation.org
TO: k...@kernel.org
TO: da...@davemloft.net
CC: linux-arm-...@vger.kernel.org
CC: aleksan...@aleksander.es
CC: linux-kernel@vger.kernel.org
CC: net...@vger.kernel.org
CC: bjorn.anders...@linaro.org
CC: manivannan.sadhasi...@linaro.org
CC: Loic Poulain 

Hi Loic,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:
https://github.com/0day-ci/linux/commits/Loic-Poulain/net-Add-a-WWAN-subsystem/20210402-220002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 
bd78980be1a68d14524c51c4b4170782fada622b
:: branch date: 7 hours ago
:: commit date: 7 hours ago
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 
Reported-by: Julia Lawall 


cocci warnings: (new ones prefixed by >>)
>> drivers/net/wwan/mhi_wwan_ctrl.c:213:17-24: ERROR: reference preceded by 
>> free on line 212

vim +213 drivers/net/wwan/mhi_wwan_ctrl.c

16d753f4f524ce Loic Poulain 2021-04-02  184
16d753f4f524ce Loic Poulain 2021-04-02  185  static int 
mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev,
16d753f4f524ce Loic Poulain 2021-04-02  186const 
struct mhi_device_id *id)
16d753f4f524ce Loic Poulain 2021-04-02  187  {
16d753f4f524ce Loic Poulain 2021-04-02  188 struct mhi_controller *cntrl = 
mhi_dev->mhi_cntrl;
16d753f4f524ce Loic Poulain 2021-04-02  189 struct mhi_wwan_dev *mhiwwan;
16d753f4f524ce Loic Poulain 2021-04-02  190
16d753f4f524ce Loic Poulain 2021-04-02  191 mhiwwan = 
kzalloc(sizeof(*mhiwwan), GFP_KERNEL);
16d753f4f524ce Loic Poulain 2021-04-02  192 if (!mhiwwan)
16d753f4f524ce Loic Poulain 2021-04-02  193 return -ENOMEM;
16d753f4f524ce Loic Poulain 2021-04-02  194
16d753f4f524ce Loic Poulain 2021-04-02  195 mhiwwan->mhi_dev = mhi_dev;
16d753f4f524ce Loic Poulain 2021-04-02  196 mhiwwan->mtu = MHI_WWAN_MAX_MTU;
16d753f4f524ce Loic Poulain 2021-04-02  197 INIT_WORK(>rx_refill, 
mhi_wwan_ctrl_refill_work);
16d753f4f524ce Loic Poulain 2021-04-02  198 
spin_lock_init(>tx_lock);
16d753f4f524ce Loic Poulain 2021-04-02  199
16d753f4f524ce Loic Poulain 2021-04-02  200 if (mhi_dev->dl_chan)
16d753f4f524ce Loic Poulain 2021-04-02  201 
set_bit(MHI_WWAN_DL_CAP, >flags);
16d753f4f524ce Loic Poulain 2021-04-02  202 if (mhi_dev->ul_chan)
16d753f4f524ce Loic Poulain 2021-04-02  203 
set_bit(MHI_WWAN_UL_CAP, >flags);
16d753f4f524ce Loic Poulain 2021-04-02  204
16d753f4f524ce Loic Poulain 2021-04-02  205 dev_set_drvdata(_dev->dev, 
mhiwwan);
16d753f4f524ce Loic Poulain 2021-04-02  206
16d753f4f524ce Loic Poulain 2021-04-02  207 /* Register as a wwan port, 
id->driver_data contains wwan port type */
16d753f4f524ce Loic Poulain 2021-04-02  208 mhiwwan->wwan_port = 
wwan_create_port(>mhi_dev->dev,
16d753f4f524ce Loic Poulain 2021-04-02  209 
  id->driver_data,
16d753f4f524ce Loic Poulain 2021-04-02  210 
  _pops, mhiwwan);
16d753f4f524ce Loic Poulain 2021-04-02  211 if (IS_ERR(mhiwwan->wwan_port)) 
{
16d753f4f524ce Loic Poulain 2021-04-02 @212 kfree(mhiwwan);
16d753f4f524ce Loic Poulain 2021-04-02 @213 return 
PTR_ERR(mhiwwan->wwan_port);
16d753f4f524ce Loic Poulain 2021-04-02  214 }
16d753f4f524ce Loic Poulain 2021-04-02  215
16d753f4f524ce Loic Poulain 2021-04-02  216 return 0;
16d753f4f524ce Loic Poulain 2021-04-02  217  };
16d753f4f524ce Loic Poulain 2021-04-02  218

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org

.config.gz
Description: application/gzip

[PATCH] iommu: dart: fix call_kern.cocci warnings

[Julia Lawall]

From: kernel test robot 

Function apple_dart_attach_stream called on line 519 inside
lock on line 509 but uses GFP_KERNEL

Generated by: scripts/coccinelle/locks/call_kern.cocci

Fixes: ce67d3b3ef37 ("iommu: dart: Add DART iommu driver")
CC: Sven Peter 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git 
hack/m1-pcie
head:   1fd2c9634dd24fba323baba52200de18e4d3f4ee
commit: ce67d3b3ef3734925631ec65deb1cf8078d826cf [14/15] iommu: dart: Add DART 
iommu driver
:: branch date: 8 hours ago
:: commit date: 8 hours ago

Please take the patch only if it's a positive warning. Thanks!

 apple-dart-iommu.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/iommu/apple-dart-iommu.c
+++ b/drivers/iommu/apple-dart-iommu.c
@@ -435,7 +435,7 @@ static int apple_dart_attach_stream(stru
goto error;
}

-   stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+   stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
if (!stream) {
ret = -ENOMEM;
goto error;

Re: [PATCH v3] coccinelle: misc: add swap script

[Julia Lawall]

On Sun, 28 Mar 2021, Denis Efremov wrote:

> Ping?

Applied.  Thanks.

>
> On 3/5/21 1:09 PM, Denis Efremov wrote:
> > Check for opencoded swap() implementation.
> >
> > Signed-off-by: Denis Efremov 
> > ---
> > Changes in v2:
> >   - additional patch rule to drop excessive {}
> >   - fix indentation in patch mode by anchoring ;
> > Changes in v3:
> >   - Rule added for simple (without var init) swap highlighting in !patch
> > mode
> >   - "depends on patch && (rpvar || rp)" fixed
> >
> >   scripts/coccinelle/misc/swap.cocci | 122 +
> >   1 file changed, 122 insertions(+)
> >   create mode 100644 scripts/coccinelle/misc/swap.cocci
> >
> > diff --git a/scripts/coccinelle/misc/swap.cocci
> > b/scripts/coccinelle/misc/swap.cocci
> > new file mode 100644
> > index ..c5e71b7ef7f5
> > --- /dev/null
> > +++ b/scripts/coccinelle/misc/swap.cocci
> > @@ -0,0 +1,122 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +///
> > +/// Check for opencoded swap() implementation.
> > +///
> > +// Confidence: High
> > +// Copyright: (C) 2021 Denis Efremov ISPRAS
> > +// Options: --no-includes --include-headers
> > +//
> > +// Keywords: swap
> > +//
> > +
> > +virtual patch
> > +virtual org
> > +virtual report
> > +virtual context
> > +
> > +@rvar depends on !patch@
> > +identifier tmp;
> > +expression a, b;
> > +type T;
> > +position p;
> > +@@
> > +
> > +(
> > +* T tmp;
> > +|
> > +* T tmp = 0;
> > +|
> > +* T *tmp = NULL;
> > +)
> > +... when != tmp
> > +* tmp = a;
> > +* a = b;@p
> > +* b = tmp;
> > +... when != tmp
> > +
> > +@r depends on !patch@
> > +identifier tmp;
> > +expression a, b;
> > +position p != rvar.p;
> > +@@
> > +
> > +* tmp = a;
> > +* a = b;@p
> > +* b = tmp;
> > +
> > +@rpvar depends on patch@
> > +identifier tmp;
> > +expression a, b;
> > +type T;
> > +@@
> > +
> > +(
> > +- T tmp;
> > +|
> > +- T tmp = 0;
> > +|
> > +- T *tmp = NULL;
> > +)
> > +... when != tmp
> > +- tmp = a;
> > +- a = b;
> > +- b = tmp
> > ++ swap(a, b)
> > +  ;
> > +... when != tmp
> > +
> > +@rp depends on patch@
> > +identifier tmp;
> > +expression a, b;
> > +@@
> > +
> > +- tmp = a;
> > +- a = b;
> > +- b = tmp
> > ++ swap(a, b)
> > +  ;
> > +
> > +@depends on patch && (rpvar || rp)@
> > +@@
> > +
> > +(
> > +  for (...;...;...)
> > +- {
> > +   swap(...);
> > +- }
> > +|
> > +  while (...)
> > +- {
> > +   swap(...);
> > +- }
> > +|
> > +  if (...)
> > +- {
> > +   swap(...);
> > +- }
> > +)
> > +
> > +
> > +@script:python depends on report@
> > +p << r.p;
> > +@@
> > +
> > +coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
> > +
> > +@script:python depends on org@
> > +p << r.p;
> > +@@
> > +
> > +coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
> > +
> > +@script:python depends on report@
> > +p << rvar.p;
> > +@@
> > +
> > +coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
> > +
> > +@script:python depends on org@
> > +p << rvar.p;
> > +@@
> > +
> > +coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
> >
>

Re: [Outreachy kernel] [PATCH] staging: rtl8188eu: core: add comma within a comment

[Julia Lawall]

On Sat, 3 Apr 2021, Deborah Brouwer wrote:

> On Sat, Apr 03, 2021 at 11:16:16PM +0200, Julia Lawall wrote:
> >
> >
> > On Sat, 3 Apr 2021, Deborah Brouwer wrote:
> >
> > > Add a comma to separate repeated words in a comment. The comma preserves
> > > the meaning of the comment while also stopping the checkpatch warning:
> > > WARNING: Possible repeated word: 'very'.
> >
> > Thanks.  That is more understandable.  Isn't this a v2?  If so, there
> > should be v2 after PATCH and an explanation of the change under the ---
> >
> > julia
> Hi Julia, no this is not a v2.  I found the same comment, generating the same
> error, in a different file.  Since I changed it in
> rtl8723bs/core/rtw_xmit.c [1], I thought I should be consistent and change
> it here as well.

OK, thanks for the explanation :)

julia

>
> [1] 
> https://lore.kernel.org/r/2944d1a0e8769edb489bb336423625a61d314d05.1617229359.git.deborahbrouwer3...@gmail.com
>
> >
> >
> > >
> > > Signed-off-by: Deborah Brouwer 
> > > ---
> > >  drivers/staging/rtl8188eu/core/rtw_xmit.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/rtl8188eu/core/rtw_xmit.c 
> > > b/drivers/staging/rtl8188eu/core/rtw_xmit.c
> > > index ed81cbc5e191..99e44b2c6f36 100644
> > > --- a/drivers/staging/rtl8188eu/core/rtw_xmit.c
> > > +++ b/drivers/staging/rtl8188eu/core/rtw_xmit.c
> > > @@ -1243,7 +1243,7 @@ s32 rtw_free_xmitbuf(struct xmit_priv *pxmitpriv, 
> > > struct xmit_buf *pxmitbuf)
> > >   * If we turn on USE_RXTHREAD, then, no need for critical section.
> > >   * Otherwise, we must use _enter/_exit critical to protect 
> > > free_xmit_queue...
> > >   *
> > > - * Must be very very cautious...
> > > + * Must be very, very cautious...
> > >   *
> > >   */
> > >
> > > --
> > > 2.17.1
> > >
> > > --
> > > 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+unsubscr...@googlegroups.com.
> > > To view this discussion on the web visit 
> > > https://groups.google.com/d/msgid/outreachy-kernel/20210403210930.17781-1-deborahbrouwer3563%40gmail.com.
> > >
>

Re: [Outreachy kernel] [PATCH] staging: rtl8188eu: replace goto with direct return

[Julia Lawall]

On Sat, 3 Apr 2021, Deborah Brouwer wrote:

> To conform with Linux kernel coding style, replace goto statement that
> does no cleanup with a direct return.  To preserve meaning, copy comments
> from the original goto statement to the return statement.  Identified by
> the checkpatch warning: WARNING: void function return statements are not
> generally useful.

Maybe the comments are meant as TODO items?

julia

>
> Signed-off-by: Deborah Brouwer 
> ---
>  drivers/staging/rtl8188eu/hal/rtl8188e_dm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8188eu/hal/rtl8188e_dm.c 
> b/drivers/staging/rtl8188eu/hal/rtl8188e_dm.c
> index 391c59490718..d21f21857c20 100644
> --- a/drivers/staging/rtl8188eu/hal/rtl8188e_dm.c
> +++ b/drivers/staging/rtl8188eu/hal/rtl8188e_dm.c
> @@ -139,7 +139,9 @@ void rtw_hal_dm_watchdog(struct adapter *Adapter)
>   hw_init_completed = Adapter->hw_init_completed;
>
>   if (!hw_init_completed)
> - goto skip_dm;
> + /*  Check GPIO to determine current RF on/off and Pbc status. */
> + /*  Check Hardware Radio ON/OFF or not */
> + return;
>
>   /* ODM */
>   pmlmepriv = >mlmepriv;
> @@ -156,10 +158,8 @@ void rtw_hal_dm_watchdog(struct adapter *Adapter)
>
>   Adapter->HalData->odmpriv.bLinked = bLinked;
>   ODM_DMWatchdog(>HalData->odmpriv);
> -skip_dm:
>   /*  Check GPIO to determine current RF on/off and Pbc status. */
>   /*  Check Hardware Radio ON/OFF or not */
> - return;
>  }
>
>  void rtw_hal_dm_init(struct adapter *Adapter)
> --
> 2.17.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210404054008.23525-1-deborahbrouwer3563%40gmail.com.
>

Re: [Outreachy kernel] [PATCH] staging: rtl8188eu: core: add comma within a comment

[Julia Lawall]

On Sat, 3 Apr 2021, Deborah Brouwer wrote:

> Add a comma to separate repeated words in a comment. The comma preserves
> the meaning of the comment while also stopping the checkpatch warning:
> WARNING: Possible repeated word: 'very'.

Thanks.  That is more understandable.  Isn't this a v2?  If so, there
should be v2 after PATCH and an explanation of the change under the ---

julia


>
> Signed-off-by: Deborah Brouwer 
> ---
>  drivers/staging/rtl8188eu/core/rtw_xmit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8188eu/core/rtw_xmit.c 
> b/drivers/staging/rtl8188eu/core/rtw_xmit.c
> index ed81cbc5e191..99e44b2c6f36 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_xmit.c
> @@ -1243,7 +1243,7 @@ s32 rtw_free_xmitbuf(struct xmit_priv *pxmitpriv, 
> struct xmit_buf *pxmitbuf)
>   * If we turn on USE_RXTHREAD, then, no need for critical section.
>   * Otherwise, we must use _enter/_exit critical to protect free_xmit_queue...
>   *
> - * Must be very very cautious...
> + * Must be very, very cautious...
>   *
>   */
>
> --
> 2.17.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210403210930.17781-1-deborahbrouwer3563%40gmail.com.
>

[PATCH] drm/amdgpu: fix semicolon.cocci warnings

[Julia Lawall]

From: kernel test robot 

Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

Fixes: 37439a51ff17 ("drm/amdgpu: Add mode2 reset support for aldebaran")
CC: Lijo Lazar 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

tree:   https://gitlab.freedesktop.org/agd5f/linux.git drm-next-5.13
head:   ef95d2a98d642a537190d73c45ae3c308afee890
commit: 37439a51ff171f938f886d6078802926fb27ccf8 [100/149] drm/amdgpu: Add 
mode2 reset support for aldebaran
:: branch date: 14 hours ago
:: commit date: 4 days ago

 aldebaran.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/gpu/drm/amd/amdgpu/aldebaran.c
+++ b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
@@ -227,7 +227,7 @@ static int aldebaran_mode2_restore_ip(st
break;
default:
break;
-   };
+   }
}

/* Reinit NBIF block */

Re: [Outreachy kernel] [PATCH] staging: emxx_udc: Ending line with argument

[Julia Lawall]

On Thu, 1 Apr 2021, Beatriz Martins de Carvalho wrote:

> Cleans up check of "Lines should not end with a '('"
> with argument present in next line in file emxx_udc.c

The coding style documentation encourages remaining within 80 characters.
I'm not sure that the ( warning is worth increading the excess beyond 80
characters (or introducing it in the last case).

julia

>
> Signed-off-by: Beatriz Martins de Carvalho 
> 
> ---
>  drivers/staging/emxx_udc/emxx_udc.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/emxx_udc/emxx_udc.c 
> b/drivers/staging/emxx_udc/emxx_udc.c
> index 741147a4f0fe..20f53cf6e20f 100644
> --- a/drivers/staging/emxx_udc/emxx_udc.c
> +++ b/drivers/staging/emxx_udc/emxx_udc.c
> @@ -1073,9 +1073,8 @@ static int _nbu2ss_epn_in_pio(struct nbu2ss_udc *udc, 
> struct nbu2ss_ep *ep,
>   i_word_length = length / sizeof(u32);
>   if (i_word_length > 0) {
>   for (i = 0; i < i_word_length; i++) {
> - _nbu2ss_writel(
> - >EP_REGS[ep->epnum - 1].EP_WRITE,
> - p_buf_32->dw);
> + _nbu2ss_writel(>EP_REGS[ep->epnum - 
> 1].EP_WRITE,
> +p_buf_32->dw);
>
>   p_buf_32++;
>   }
> @@ -1225,8 +1224,7 @@ static void _nbu2ss_restert_transfer(struct nbu2ss_ep 
> *ep)
>   return;
>
>   if (ep->epnum > 0) {
> - length = _nbu2ss_readl(
> - >udc->p_regs->EP_REGS[ep->epnum - 1].EP_LEN_DCNT);
> + length = _nbu2ss_readl(>udc->p_regs->EP_REGS[ep->epnum - 
> 1].EP_LEN_DCNT);
>
>   length &= EPN_LDATA;
>   if (length < ep->ep.maxpacket)
> @@ -1462,8 +1460,7 @@ static void _nbu2ss_epn_set_stall(struct nbu2ss_udc 
> *udc,
>   for (limit_cnt = 0
>   ; limit_cnt < IN_DATA_EMPTY_COUNT
>   ; limit_cnt++) {
> - regdata = _nbu2ss_readl(
> - >EP_REGS[ep->epnum - 1].EP_STATUS);
> + regdata = _nbu2ss_readl(>EP_REGS[ep->epnum - 
> 1].EP_STATUS);
>
>   if ((regdata & EPN_IN_DATA) == 0)
>   break;
> --
> 2.25.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210401195457.24512-1-martinsdecarvalhobeatriz%40gmail.com.
>

Re: [Outreachy kernel] [PATCH] staging: greybus: arche-platform: Fix ending '(' warnings

[Julia Lawall]

On Wed, 31 Mar 2021, Beatriz Martins de Carvalho wrote:

> Fix checkpatch check "CHECK: Lines should not end with a '('"
> in arche-platform.c:80 and arche-platform.c:184.

Please try to express what you have done and why, without using the word
Fix.  "Fix" doesn't explain what you have done.  It just says that you
have done something that you consider has made the code better.  But the
person receiving the message may have something else in mind.

julia

>
> Signed-off-by: Beatriz Martins de Carvalho 
> 
> ---
>  drivers/staging/greybus/arche-platform.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/greybus/arche-platform.c 
> b/drivers/staging/greybus/arche-platform.c
> index eebf0deb39f5..e374dfc0c92f 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -77,9 +77,8 @@ static void arche_platform_set_state(struct 
> arche_platform_drvdata *arche_pdata,
>  }
>
>  /* Requires arche_pdata->wake_lock is held by calling context */
> -static void arche_platform_set_wake_detect_state(
> - struct arche_platform_drvdata *arche_pdata,
> - enum svc_wakedetect_state state)
> +static void arche_platform_set_wake_detect_state(struct 
> arche_platform_drvdata *arche_pdata,
> +  enum svc_wakedetect_state 
> state)
>  {
>   arche_pdata->wake_detect_state = state;
>  }
> @@ -181,9 +180,8 @@ static irqreturn_t arche_platform_wd_irq(int irq, void 
> *devid)
>   WD_STATE_COLDBOOT_START) {
>   
> arche_platform_set_wake_detect_state(arche_pdata,
>
> WD_STATE_COLDBOOT_TRIG);
> - spin_unlock_irqrestore(
> - _pdata->wake_lock,
> - flags);
> + 
> spin_unlock_irqrestore(_pdata->wake_lock,
> +flags);
>   return IRQ_WAKE_THREAD;
>   }
>   }
> --
> 2.25.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20210331202445.108678-1-martinsdecarvalhobeatriz%40gmail.com.
>

Re: [Outreachy kernel] [PATCH 1/3] staging: rtl8723bs: core: fix repeated word warning

[Julia Lawall]

On Wed, 31 Mar 2021, Deborah Brouwer wrote:

> Fix checkpatch warning:
> WARNING: Possible repeated word: 'very'

This is a simple but clear example of how "Fix" doesn't help one
understand the patch.  In reading the log message, one would probably
assume that you removed the repetition, but actually you added a comma.
So it would be better to explain what you did and why.  It's good to
acknowledge checkpatch, but the text of the warning message is not that
useful.

julia

>
> Signed-off-by: Deborah Brouwer 
> ---
>  drivers/staging/rtl8723bs/core/rtw_xmit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c 
> b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 2daf5c461a4d..3878caf0b56c 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> @@ -1700,7 +1700,7 @@ Calling context:
>  If we turn on USE_RXTHREAD, then, no need for critical section.
>  Otherwise, we must use _enter/_exit critical to protect free_xmit_queue...
>
> -Must be very very cautious...
> +Must be very, very cautious...
>
>  */
>  struct xmit_frame *rtw_alloc_xmitframe(struct xmit_priv *pxmitpriv)/* _queue 
> *pfree_xmit_queue) */
> --
> 2.17.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/14c1a1f1f0a34fb821d47ddab6e7e63800ec2736.1617221075.git.deborahbrouwer3563%40gmail.com.
>

Re: [Outreachy kernel] [PATCH 2/3] staging: rtl8723bs: core: fix block comment warning

[Julia Lawall]

On Wed, 31 Mar 2021, Deborah Brouwer wrote:

> Fix checkpatch warning:
> WARNING: Block comments use * on subsequent lines

Try to find some way to express what you are doing without using Fix.
It's pretty obvious that a patch fixes something, so the work Fix is not
very useful.  What is your fix and why have you chosen to do that?
Something like the following would be more informative.

Add *s at the begiinning of each line in a block comment to conform to the
Linux kernel coding style.  Issue detected using checkpatch.

julia

>
> Signed-off-by: Deborah Brouwer 
> ---
>  drivers/staging/rtl8723bs/core/rtw_xmit.c | 59 +++
>  1 file changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c 
> b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 3878caf0b56c..428c71ce0334 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> @@ -46,8 +46,8 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct 
> adapter *padapter)
>   init_completion(>terminate_xmitthread_comp);
>
>   /*
> - Please insert all the queue initializaiton using _rtw_init_queue below
> - */
> +  * Please insert all the queue initializaiton using _rtw_init_queue 
> below
> +  */
>
>   pxmitpriv->adapter = padapter;
>
> @@ -60,10 +60,10 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, 
> struct adapter *padapter)
>   _rtw_init_queue(>free_xmit_queue);
>
>   /*
> - Please allocate memory with the sz = (struct xmit_frame) * NR_XMITFRAME,
> - and initialize free_xmit_frame below.
> - Please also apply  free_txobj to link_up all the xmit_frames...
> - */
> +  * Please allocate memory with the sz = (struct xmit_frame) * 
> NR_XMITFRAME,
> +  * and initialize free_xmit_frame below.
> +  * Please also apply  free_txobj to link_up all the xmit_frames...
> +  */
>
>   pxmitpriv->pallocated_frame_buf = vzalloc(NR_XMITFRAME * sizeof(struct 
> xmit_frame) + 4);
>
> @@ -1069,17 +1069,15 @@ u32 rtw_calculate_wlan_pkt_size_by_attribue(struct 
> pkt_attrib *pattrib)
>  }
>
>  /*
> -
> -This sub-routine will perform all the following:
> -
> -1. remove 802.3 header.
> -2. create wlan_header, based on the info in pxmitframe
> -3. append sta's iv/ext-iv
> -4. append LLC
> -5. move frag chunk from pframe to pxmitframe->mem
> -6. apply sw-encrypt, if necessary.
> -
> -*/
> + * This sub-routine will perform all the following:
> + *
> + * 1. remove 802.3 header.
> + * 2. create wlan_header, based on the info in pxmitframe
> + * 3. append sta's iv/ext-iv
> + * 4. append LLC
> + * 5. move frag chunk from pframe to pxmitframe->mem
> + * 6. apply sw-encrypt, if necessary.
> + */
>  s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt, 
> struct xmit_frame *pxmitframe)
>  {
>   struct pkt_file pktfile;
> @@ -1693,23 +1691,22 @@ static void rtw_init_xmitframe(struct xmit_frame 
> *pxframe)
>  }
>
>  /*
> -Calling context:
> -1. OS_TXENTRY
> -2. RXENTRY (rx_thread or RX_ISR/RX_CallBack)
> -
> -If we turn on USE_RXTHREAD, then, no need for critical section.
> -Otherwise, we must use _enter/_exit critical to protect free_xmit_queue...
> -
> -Must be very, very cautious...
> -
> -*/
> + * Calling context:
> + * 1. OS_TXENTRY
> + * 2. RXENTRY (rx_thread or RX_ISR/RX_CallBack)
> + *
> + * If we turn on USE_RXTHREAD, then, no need for critical section.
> + * Otherwise, we must use _enter/_exit critical to protect free_xmit_queue...
> + *
> + * Must be very, very cautious...
> + */
>  struct xmit_frame *rtw_alloc_xmitframe(struct xmit_priv *pxmitpriv)/* _queue 
> *pfree_xmit_queue) */
>  {
>   /*
> - Please remember to use all the osdep_service api,
> - and lock/unlock or _enter/_exit critical to protect
> - pfree_xmit_queue
> - */
> +  *  Please remember to use all the osdep_service api,
> +  *  and lock/unlock or _enter/_exit critical to protect
> +  *  pfree_xmit_queue
> +  */
>
>   struct xmit_frame *pxframe = NULL;
>   struct list_head *plist, *phead;
> --
> 2.17.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/5ff15315036be9bdf2059bab2ddd00f7dce0d20a.1617221075.git.deborahbrouwer3563%40gmail.com.
>

Re: [Outreachy kernel] [PATCH 3/3] staging: rtl8723bs: core: remove empty comment

[Julia Lawall]

On Wed, 31 Mar 2021, Deborah Brouwer wrote:

> Remove empty comment instead of fixing the checkpatch warning that it was
> generating.

Maybe it woudl be better to focus on the purpose of what you are doing.
Something like:

Remove empty comment, which provides no information.

julia

>
> Signed-off-by: Deborah Brouwer 
> ---
>  drivers/staging/rtl8723bs/core/rtw_xmit.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c 
> b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 428c71ce0334..7b4c0f22cd90 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> @@ -876,8 +876,6 @@ static s32 xmitframe_addmic(struct adapter *padapter, 
> struct xmit_frame *pxmitfr
>   *(payload+curfragnum), 
> *(payload+curfragnum+1), *(payload+curfragnum+2), *(payload+curfragnum+3),
>   *(payload+curfragnum+4), 
> *(payload+curfragnum+5), *(payload+curfragnum+6), *(payload+curfragnum+7)));
>   }
> -/*
> -*/
>   }
>   return _SUCCESS;
>  }
> --
> 2.17.1
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/f6be2f84ff5b29a51ca6be0d269c50d54568b2e2.1617221075.git.deborahbrouwer3563%40gmail.com.
>

[PATCH] ASoC: cygnus: fix for_each_child.cocci warnings

[Julia Lawall]

From: kernel test robot 

Function "for_each_available_child_of_node" should have of_node_put()
before return around line 1352.

Generated by: scripts/coccinelle/iterators/for_each_child.cocci

CC: Sumera Priyadarsini 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

 cygnus-ssp.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- a/sound/soc/bcm/cygnus-ssp.c
+++ b/sound/soc/bcm/cygnus-ssp.c
@@ -1348,8 +1348,10 @@ static int cygnus_ssp_probe(struct platf
_ssp_dai[active_port_count]);

/* negative is err, 0 is active and good, 1 is disabled */
-   if (err < 0)
+   if (err < 0) {
+   of_node_put(child_node);
return err;
+   }
else if (!err) {
dev_dbg(dev, "Activating DAI: %s\n",
cygnus_ssp_dai[active_port_count].name);

[PATCH] sit: use min

[Julia Lawall]

From: kernel test robot 

Opportunity for min()

Generated by: scripts/coccinelle/misc/minmax.cocci

CC: Denis Efremov 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
---

 sit.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -325,7 +325,7 @@ static int ipip6_tunnel_get_prl(struct n

rcu_read_lock();

-   ca = t->prl_count < cmax ? t->prl_count : cmax;
+   ca = min(t->prl_count, cmax);

if (!kp) {
/* We don't try hard to allocate much memory for

Re: [PATCH] coccinelle: misc: update uninitialized_var.cocci documentation

[Julia Lawall]

On Mon, 8 Mar 2021, Denis Efremov wrote:

> Remove the documentation link from the warning message because commit
> 3942ea7a10c9 ("deprecated.rst: Remove now removed uninitialized_var")
> removed the section from documentation. Update the rule documentation
> accordingly.
>
> Signed-off-by: Denis Efremov 

Applied, thanks.

julia

> ---
>  scripts/coccinelle/misc/uninitialized_var.cocci | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/coccinelle/misc/uninitialized_var.cocci 
> b/scripts/coccinelle/misc/uninitialized_var.cocci
> index 8fa845cefe11..69bbaae47e73 100644
> --- a/scripts/coccinelle/misc/uninitialized_var.cocci
> +++ b/scripts/coccinelle/misc/uninitialized_var.cocci
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  ///
>  /// Please, don't reintroduce uninitialized_var().
> -/// From Documentation/process/deprecated.rst:
> +///
> +/// From Documentation/process/deprecated.rst,
> +/// commit 4b19bec97c88 ("docs: deprecated.rst: Add uninitialized_var()"):
>  ///  For any compiler warnings about uninitialized variables, just add
>  ///  an initializer. Using warning-silencing tricks is dangerous as it
>  ///  papers over real bugs (or can in the future), and suppresses unrelated
> @@ -11,6 +13,11 @@
>  ///  obviously redundant, the compiler's dead-store elimination pass will 
> make
>  ///  sure there are no needless variable writes.
>  ///
> +/// Later, commit 3942ea7a10c9 ("deprecated.rst: Remove now removed
> +/// uninitialized_var") removed this section because all initializations of
> +/// this kind were cleaned-up from the kernel. This cocci rule checks that
> +/// the macro is not explicitly or implicitly reintroduced.
> +///
>  // Confidence: High
>  // Copyright: (C) 2020 Denis Efremov ISPRAS
>  // Options: --no-includes --include-headers
> @@ -40,12 +47,10 @@ position p;
>  p << r.p;
>  @@
>
> -coccilib.report.print_report(p[0],
> -  "WARNING this kind of initialization is deprecated 
> (https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
> +coccilib.report.print_report(p[0], "WARNING this kind of initialization is 
> deprecated")
>
>  @script:python depends on org@
>  p << r.p;
>  @@
>
> -coccilib.org.print_todo(p[0],
> -  "WARNING this kind of initialization is deprecated 
> (https://www.kernel.org/doc/html/latest/process/deprecated.html#uninitialized-var)")
> +coccilib.org.print_todo(p[0], "WARNING this kind of initialization is 
> deprecated")
> --
> 2.26.2
>
>

Re: [PATCH] coccinelle: misc: restrict patch mode in flexible_array.cocci

[Julia Lawall]

On Mon, 8 Mar 2021, Denis Efremov wrote:

> Skip patches generation for structs/unions with a single field.
> Changing a zero-length array to a flexible array member in a struct
> with no named members breaks the compilation. However, reporting
> such cases is still valuable, e.g. commit 637464c59e0b
> ("ACPI: NFIT: Fix flexible_array.cocci warnings").
>
> Signed-off-by: Denis Efremov 

Applied.  I changed the log message to remove "/unions", since the change
doesn't mention unions.

julia

> ---
>  scripts/coccinelle/misc/flexible_array.cocci | 23 ++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/coccinelle/misc/flexible_array.cocci 
> b/scripts/coccinelle/misc/flexible_array.cocci
> index 947fbaff82a9..f427fd68ed2d 100644
> --- a/scripts/coccinelle/misc/flexible_array.cocci
> +++ b/scripts/coccinelle/misc/flexible_array.cocci
> @@ -51,21 +51,40 @@ position p : script:python() { relevant(p) };
>};
>  )
>
> +@only_field depends on patch@
> +identifier name, array;
> +type T;
> +position q;
> +@@
> +
> +(
> +  struct name {@q
> +T array[0];
> +  };
> +|
> +  struct {@q
> +T array[0];
> +  };
> +)
> +
>  @depends on patch@
>  identifier name, array;
>  type T;
>  position p : script:python() { relevant(p) };
> +// position @q with rule "only_field" simplifies
> +// handling of bitfields, arrays, etc.
> +position q != only_field.q;
>  @@
>
>  (
> -  struct name {
> +  struct name {@q
>  ...
>  T array@p[
>  -   0
>  ];
>};
>  |
> -  struct {
> +  struct {@q
>  ...
>  T array@p[
>  -   0
> --
> 2.26.2
>
>

[PATCH] of: overlay: fix for_each_child.cocci warnings

[Julia Lawall]

From: kernel test robot 

Function "for_each_child_of_node" should have of_node_put() before goto.

Generated by: scripts/coccinelle/iterators/for_each_child.cocci

Fixes: 82c2d81361ec ("coccinelle: iterators: Add for_each_child.cocci script")
CC: Sumera Priyadarsini 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   812da4d39463a060738008a46cfc9f775e4bfcf6
commit: 82c2d81361ecd142a54e84a9da1e287113314a4f coccinelle: iterators: Add 
for_each_child.cocci script
:: branch date: 13 hours ago
:: commit date: 5 months ago

 overlay.c |1 +
 1 file changed, 1 insertion(+)

--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -796,6 +796,7 @@ static int init_overlay_changeset(struct
if (!fragment->target) {
of_node_put(fragment->overlay);
ret = -EINVAL;
+   of_node_put(node);
goto err_free_fragments;
}

Re: [PATCH v5] coccinelle: misc: add minmax script

[Julia Lawall]

On Tue, 9 Mar 2021, Denis Efremov wrote:

> Check for opencoded min(), max() implementations.
>
> Signed-off-by: Denis Efremov 

Applied, thanks.

julia

> ---
> Changes in v2:
>  - <... ...> instead of ... when any
>  - org mode reports fixed
>  - patch rule to drop excessive ()
> Changes in v3:
>  - "depends on patch && (pmax || pmaxif || pmin || pminif)" fixed
> Changes in v4:
>  - refarmatting rule removed
>  - () brackets added to the patch rules to omit excessive ones
>  - org/report prints changed to cycle (for p0 in p: ...)
> Changes in v5:
>  - parentheses droppped in pminif and pmaxif rules (max_val = x ...)
>
>  scripts/coccinelle/misc/minmax.cocci | 206 +++
>  1 file changed, 206 insertions(+)
>  create mode 100644 scripts/coccinelle/misc/minmax.cocci
>
> diff --git a/scripts/coccinelle/misc/minmax.cocci 
> b/scripts/coccinelle/misc/minmax.cocci
> new file mode 100644
> index ..eccdd3eb3452
> --- /dev/null
> +++ b/scripts/coccinelle/misc/minmax.cocci
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Check for opencoded min(), max() implementations.
> +/// Generated patches sometimes require adding a cast to fix compile warning.
> +/// Warnings/patches scope intentionally limited to a function body.
> +///
> +// Confidence: Medium
> +// Copyright: (C) 2021 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: min, max
> +//
> +
> +
> +virtual report
> +virtual org
> +virtual context
> +virtual patch
> +
> +@rmax depends on !patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {>, >=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*((x) cmp@p (y) ? (x) : (y))
> + ...>
> +}
> +
> +@rmaxif depends on !patch@
> +identifier func;
> +expression x, y;
> +expression max_val;
> +binary operator cmp = {>, >=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*if ((x) cmp@p (y)) {
> +*max_val = (x);
> +*} else {
> +*max_val = (y);
> +*}
> + ...>
> +}
> +
> +@rmin depends on !patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {<, <=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*((x) cmp@p (y) ? (x) : (y))
> + ...>
> +}
> +
> +@rminif depends on !patch@
> +identifier func;
> +expression x, y;
> +expression min_val;
> +binary operator cmp = {<, <=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*if ((x) cmp@p (y)) {
> +*min_val = (x);
> +*} else {
> +*min_val = (y);
> +*}
> + ...>
> +}
> +
> +@pmax depends on patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {>=, >};
> +@@
> +
> +func(...)
> +{
> + <...
> +-((x) cmp (y) ? (x) : (y))
> ++max(x, y)
> + ...>
> +}
> +
> +@pmaxif depends on patch@
> +identifier func;
> +expression x, y;
> +expression max_val;
> +binary operator cmp = {>=, >};
> +@@
> +
> +func(...)
> +{
> + <...
> +-if ((x) cmp (y)) {
> +-max_val = x;
> +-} else {
> +-max_val = y;
> +-}
> ++max_val = max(x, y);
> + ...>
> +}
> +
> +@pmin depends on patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {<=, <};
> +@@
> +
> +func(...)
> +{
> + <...
> +-((x) cmp (y) ? (x) : (y))
> ++min(x, y)
> + ...>
> +}
> +
> +@pminif depends on patch@
> +identifier func;
> +expression x, y;
> +expression min_val;
> +binary operator cmp = {<=, <};
> +@@
> +
> +func(...)
> +{
> + <...
> +-if ((x) cmp (y)) {
> +-min_val = x;
> +-} else {
> +-min_val = y;
> +-}
> ++min_val = min(x, y);
> + ...>
> +}
> +
> +@script:python depends on report@
> +p << rmax.p;
> +@@
> +
> +for p0 in p:
> + coccilib.report.print_report(p0, "WARNING opportunity for max()")
> +
> +@script:python depends on org@
> +p << rmax.p;
> +@@
> +
> +for p0 in p:
> + coccilib.org.print_todo(p0, "WARNING opportunity for max()")
> +
> +@script:python depends on report@
> +p << rmaxif.p;
> +@@
> +
> +for p0 in p:
> + coccilib.report.print_report(p0, "WARNING opportunity for max()")
> +
> +@script:python depends on org@
> +p << rmaxif.p;
> +@@
> +
> +for p0 in p:
> + coccilib.org.print_todo(p0, "WARNING opportunity for max()")
> +
> +@script:python depends on report@
> +p << rmin.p;
> +@@
> +
> +for p0 in p:
> + coccilib.report.print_report(p0, "WARNING opportunity for min()")
> +
> +@script:python depends on org@
> +p << rmin.p;
> +@@
> +
> +for p0 in p:
> + coccilib.org.print_todo(p0, "WARNING opportunity for min()")
> +
> +@script:python depends on report@
> +p << rminif.p;
> +@@
> +
> +for p0 in p:
> + coccilib.report.print_report(p0, "WARNING opportunity for min()")
> +
> +@script:python depends on org@
> +p << rminif.p;
> +@@
> +
> +for p0 in p:
> + coccilib.org.print_todo(p0, "WARNING opportunity for min()")
> --
> 2.26.2
>
>

Re: [PATCH] scripts/coccinelle: Add script to detect sign extension

[Julia Lawall]

On Fri, 19 Mar 2021, Evan Benn wrote:

> Hello,
>
> I am attempting to create a coccinelle script that will detect possibly buggy
> usage of the bitwise operators where integer promotion may result in bugs,
> usually due to sign extension.
>
> I know this script needs a lot more work, but I am just beginning to learn the
> syntax of coccinelle. At this stage I am mainly looking for advice if this is
> even worth continuing, or if I am on the wrong track entirely.

I'm not really an expert in the problem, so I don't know exactly what are
the kinds of code you want to find.  Coccinelle is good at matching the
types of things and the structure of things.  If you need to know the
actual values of things, you may want to try smatch.  Coccinelle probably
doesn't have complete knowledge of how various operators affect C types.
For example, it would not have known that BIT results in a long.

The best you can do is try some rules and see what the results are, and
try to collect some relevant examples and see if you can match them with
your rules.  Please write back if there is some specific code that is not
matched as expected.

julia


>
> Here is an example of the bug I hope to find:
>
> https://lore.kernel.org/lkml/20210317013758.ga134...@roeck-us.net/
>
> Where ints and unsigned are mixed in bitwise operations, and the sizes differ.
>
> Thanks
>
> Evan Benn
>
> Signed-off-by: Evan Benn 
> ---
>
>  .../coccinelle/tests/int_sign_extend.cocci| 35 +++
>  1 file changed, 35 insertions(+)
>  create mode 100644 scripts/coccinelle/tests/int_sign_extend.cocci
>
> diff --git a/scripts/coccinelle/tests/int_sign_extend.cocci 
> b/scripts/coccinelle/tests/int_sign_extend.cocci
> new file mode 100644
> index ..bad61e37e4e7
> --- /dev/null
> +++ b/scripts/coccinelle/tests/int_sign_extend.cocci
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/// Mixing signed and unsigned types in bitwise operations risks problems 
> when
> +/// the 'Usual arithmetic conversions' are applied.
> +/// For example:
> +/// https://lore.kernel.org/lkml/20210317013758.ga134...@roeck-us.net/
> +/// When a signed int and an unsigned int are compared there is no problem.
> +/// But if the unsigned is changed to a unsigned long, for example by using 
> BIT
> +/// the signed value will be sign-extended and could result in incorrect 
> logic.
> +// Confidence:
> +// Copyright: (C) 2021 Evan Benn 
> +// Comments:
> +// Options:
> +
> +virtual context
> +virtual org
> +virtual report
> +
> +@r@
> +position p;
> +{int} s;
> +{unsigned long} u;
> +@@
> +s@p & u
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +cocci.print_main("sign extension when comparing bits of signed and unsigned 
> values", p)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +coccilib.report.print_report(p[0],"sign extension when comparing bits of 
> signed and unsigned values")
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
>

[PATCH] asus-laptop: fix kobj_to_dev.cocci warnings

[Julia Lawall]

From: kernel test robot 

Use kobj_to_dev() instead of container_of()

Generated by: scripts/coccinelle/api/kobj_to_dev.cocci

CC: Denis Efremov 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   1df27313f50a57497c1faeb6a6ae4ca939c85a7d
commit: a2fc3718bc22e85378085568ecc5765fb28cabce coccinelle: api: add 
kobj_to_dev.cocci script
:: branch date: 5 hours ago
:: commit date: 7 months ago

 asus-laptop.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -1569,7 +1569,7 @@ static umode_t asus_sysfs_is_visible(str
struct attribute *attr,
int idx)
 {
-   struct device *dev = container_of(kobj, struct device, kobj);
+   struct device *dev = kobj_to_dev(kobj);
struct asus_laptop *asus = dev_get_drvdata(dev);
acpi_handle handle = asus->handle;
bool supported;

[PATCH v2] tty: max310x: fix flexible_array.cocci warnings

[Julia Lawall]

From: kernel test robot 

Zero-length and one-element arrays are deprecated, see
Documentation/process/deprecated.rst
Flexible-array members should be used instead.

Generated by: scripts/coccinelle/misc/flexible_array.cocci

Fixes: 10d8b34a42171 ("serial: max310x: Driver rework")
CC: Denis Efremov 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

v2: reference the correct commit for Fixes

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
commit: 7b36c1398fb63f9c38cc83dc75f143d2e5995062 coccinelle: misc: add 
flexible_array.cocci script
:: branch date: 6 hours ago
:: commit date: 5 months ago

 max310x.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -273,7 +273,7 @@ struct max310x_port {
 #ifdef CONFIG_GPIOLIB
struct gpio_chipgpio;
 #endif
-   struct max310x_one  p[0];
+   struct max310x_one  p[];
 };

 static struct uart_driver max310x_uart = {

Re: [PATCH] tty: max310x: fix flexible_array.cocci warnings

[Julia Lawall]

On Tue, 9 Mar 2021, Greg Kroah-Hartman wrote:

> On Mon, Mar 08, 2021 at 06:47:19PM +0100, Julia Lawall wrote:
> > From: kernel test robot 
> >
> > Zero-length and one-element arrays are deprecated, see
> > Documentation/process/deprecated.rst
> > Flexible-array members should be used instead.
> >
> > Generated by: scripts/coccinelle/misc/flexible_array.cocci
> >
> > Fixes: 7b36c1398fb6 ("coccinelle: misc: add flexible_array.cocci script")
>
> This can't be true, as the driver code did not change for that commit :(
>
> Can you provide the commit where the driver itself caused the problem?

Sorry, 0-day seems to have a problem for the subject lines in some cases,
and it seems that it has a problem for the Fixes lines as well.  I will
find the proper one.

julia

Re: [PATCH v4] coccinelle: misc: add minmax script

[Julia Lawall]

> +@pmaxif depends on patch@
> +identifier func;
> +expression x, y;
> +expression max_val;
> +binary operator cmp = {>=, >};
> +@@
> +
> +func(...)
> +{
> + <...
> +-if ((x) cmp (y)) {
> +-max_val = (x);
> +-} else {
> +-max_val = (y);
> +-}
> ++max_val = max(x, y);
> + ...>
> +}

Things work better if there are no parentheses in max_val = (x) and
max_val = (y).  Leaving them there seems to cause the match to work in two
ways, causing an already tagged token error.  An example is in
crypto/jitterentropy.c

The same is true of the pminif rule.  Only the patch rules are affected.
Double matches are allowed in the context cas, ince there is no real
transfotmation in that case.

julia

> +
> +@pmin depends on patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {<=, <};
> +@@
> +
> +func(...)
> +{
> + <...
> +-((x) cmp (y) ? (x) : (y))
> ++min(x, y)
> + ...>
> +}
> +
> +@pminif depends on patch@
> +identifier func;
> +expression x, y;
> +expression min_val;
> +binary operator cmp = {<=, <};
> +@@
> +
> +func(...)
> +{
> + <...
> +-if ((x) cmp (y)) {
> +-min_val = (x);
> +-} else {
> +-min_val = (y);
> +-}
> ++min_val = min(x, y);
> + ...>
> +}
> +
> +@script:python depends on report@
> +p << rmax.p;
> +@@
> +
> +for p0 in p:
> + coccilib.report.print_report(p0, "WARNING opportunity for max()")
> +
> +@script:python depends on org@
> +p << rmax.p;
> +@@
> +
> +for p0 in p:
> + coccilib.org.print_todo(p0, "WARNING opportunity for max()")
> +
> +@script:python depends on report@
> +p << rmaxif.p;
> +@@
> +
> +for p0 in p:
> + coccilib.report.print_report(p0, "WARNING opportunity for max()")
> +
> +@script:python depends on org@
> +p << rmaxif.p;
> +@@
> +
> +for p0 in p:
> + coccilib.org.print_todo(p0, "WARNING opportunity for max()")
> +
> +@script:python depends on report@
> +p << rmin.p;
> +@@
> +
> +for p0 in p:
> + coccilib.report.print_report(p0, "WARNING opportunity for min()")
> +
> +@script:python depends on org@
> +p << rmin.p;
> +@@
> +
> +for p0 in p:
> + coccilib.org.print_todo(p0, "WARNING opportunity for min()")
> +
> +@script:python depends on report@
> +p << rminif.p;
> +@@
> +
> +for p0 in p:
> + coccilib.report.print_report(p0, "WARNING opportunity for min()")
> +
> +@script:python depends on org@
> +p << rminif.p;
> +@@
> +
> +for p0 in p:
> + coccilib.org.print_todo(p0, "WARNING opportunity for min()")
> --
> 2.26.2
>
>

[PATCH] tty: max310x: fix flexible_array.cocci warnings

[Julia Lawall]

From: kernel test robot 

Zero-length and one-element arrays are deprecated, see
Documentation/process/deprecated.rst
Flexible-array members should be used instead.

Generated by: scripts/coccinelle/misc/flexible_array.cocci

Fixes: 7b36c1398fb6 ("coccinelle: misc: add flexible_array.cocci script")
CC: Denis Efremov 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
Signed-off-by: Julia Lawall 
---

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
commit: 7b36c1398fb63f9c38cc83dc75f143d2e5995062 coccinelle: misc: add 
flexible_array.cocci script
:: branch date: 6 hours ago
:: commit date: 5 months ago

 max310x.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -273,7 +273,7 @@ struct max310x_port {
 #ifdef CONFIG_GPIOLIB
struct gpio_chipgpio;
 #endif
-   struct max310x_one  p[0];
+   struct max310x_one  p[];
 };

 static struct uart_driver max310x_uart = {

Re: linux-kernel janitorial RFP: Mark static arrays as const

[Julia Lawall]

On Sun, 7 Mar 2021, Joe Perches wrote:

> On Sun, 2021-03-07 at 20:14 +0100, Julia Lawall wrote:
> >
> > On Wed, 3 Mar 2021, Joe Perches wrote:
> >
> > > On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> > > > On 02/03/2021 18.42, Joe Perches wrote:
> > > > > Here is a possible opportunity to reduce data usage in the kernel.
> > > > >
> > > > > $ git grep -P -n 
> > > > > '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
> > > > >   grep -v __initdata | \
> > > > >   wc -l
> > > > > 3250
> > > > >
> > > > > Meaning there are ~3000 declarations of arrays with what appears to be
> > > > > file static const content that are not marked const.
> > > > >
> > > > > So there are many static arrays that could be marked const to move the
> > > > > compiled object code from data to text minimizing the total amount of
> > > > > exposed r/w data.
> > > >
> > > > You can add const if you like, but it will rarely change the generated
> > > > code. gcc is already smart enough to take a static array whose contents
> > > > are provably never modified within the TU and put it in .rodata:
> > >
> > > At least some or perhaps even most of the time, true, but the gcc compiler
> > > from v5 through at least v10 seems inconsistent about when it does the
> > > appropriate conversion.
> > >
> > > See the example I posted:
> > > https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.ca...@perches.com/
> > >
> > > It was a randomly chosen source file conversion btw, I had no prior
> > > knowledge of whether the text/data use would change.
> > >
> > > I'm unsure about clang consistently moving static but provably const 
> > > arrays
> > > from data to text.  I rarely use clang.  At least for v11 it seems to be
> > > better though.  I didn't try 10.1.
> >
> > I tried the relevnt drivers in drivers/input/joystick.  I got only one
> > driver that changed with gcc 9.3, which was
> > drivers/input/joystick/analog.c.  It actually got larger:
> >
> > original:
> >
> >    textdata bss dec hex filename
> >   22607   10560 320   3348782cf drivers/input/joystick/analog.o
> >
> > after adding const:
> >
> >    textdata bss dec hex filename
> >   22728   10816 320   338648448 drivers/input/joystick/analog.o
> >
> > This was the only case where bss was not 0, but I don't know if there is a
> > connection.
>
> You really need consider using defconfig so whatever object code
> does not have tracing/debugging support.
>
> For instance, this code with defconfig and analog joystick:
>
> Original:
>
> $ size drivers/input/joystick/analog.o
>text  data bss dec hex filename
>8115   261 22486002198 drivers/input/joystick/analog.o
>
> with const:
>
> $ size drivers/input/joystick/analog.o
>text  data bss dec hex filename
>8179   201 2248604219c drivers/input/joystick/analog.o

Thanks for the suggestion.  It occurred to me that in one case my
transformation was wrong, because the array was multi-level, and a sub
array was being stored in a structure field that was not declared as
const.  So the argument that the compiler would figure out to put the
array in .rodata didn't make sense.  But I still go the same sizes for
that file.  So I'll try the whole thing again.

thanks,
julia

Re: linux-kernel janitorial RFP: Mark static arrays as const

[Julia Lawall]

On Wed, 3 Mar 2021, Joe Perches wrote:

> On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> > On 02/03/2021 18.42, Joe Perches wrote:
> > > Here is a possible opportunity to reduce data usage in the kernel.
> > >
> > > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' 
> > > drivers/ | \
> > >   grep -v __initdata | \
> > >   wc -l
> > > 3250
> > >
> > > Meaning there are ~3000 declarations of arrays with what appears to be
> > > file static const content that are not marked const.
> > >
> > > So there are many static arrays that could be marked const to move the
> > > compiled object code from data to text minimizing the total amount of
> > > exposed r/w data.
> >
> > You can add const if you like, but it will rarely change the generated
> > code. gcc is already smart enough to take a static array whose contents
> > are provably never modified within the TU and put it in .rodata:
>
> At least some or perhaps even most of the time, true, but the gcc compiler
> from v5 through at least v10 seems inconsistent about when it does the
> appropriate conversion.
>
> See the example I posted:
> https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.ca...@perches.com/
>
> It was a randomly chosen source file conversion btw, I had no prior
> knowledge of whether the text/data use would change.
>
> I'm unsure about clang consistently moving static but provably const arrays
> from data to text.  I rarely use clang.  At least for v11 it seems to be
> better though.  I didn't try 10.1.

I tried the relevnt drivers in drivers/input/joystick.  I got only one
driver that changed with gcc 9.3, which was
drivers/input/joystick/analog.c.  It actually got larger:

original:

   textdata bss dec hex filename
  22607   10560 320   3348782cf drivers/input/joystick/analog.o

after adding const:

   textdata bss dec hex filename
  22728   10816 320   338648448 drivers/input/joystick/analog.o

This was the only case where bss was not 0, but I don't know if there is a
connection.

julia

Re: [PATCH v2 RESEND] coccinelle: misc: add minmax script

[Julia Lawall]

On Fri, 19 Feb 2021, Denis Efremov wrote:

> Check for opencoded min(), max() implementations.
>
> Signed-off-by: Denis Efremov 
> ---
>
> Changes in v2:
>  - <... ...> instead of ... when any
>  - org mode reports fixed
>  - patch rule to drop excessive ()
>
>  scripts/coccinelle/misc/minmax.cocci | 224 +++
>  1 file changed, 224 insertions(+)
>  create mode 100644 scripts/coccinelle/misc/minmax.cocci
>
> diff --git a/scripts/coccinelle/misc/minmax.cocci 
> b/scripts/coccinelle/misc/minmax.cocci
> new file mode 100644
> index ..61d6b61fd82c
> --- /dev/null
> +++ b/scripts/coccinelle/misc/minmax.cocci
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Check for opencoded min(), max() implementations.
> +/// Generated patches sometimes require adding a cast to fix compile warning.
> +/// Warnings/patches scope intentionally limited to a function body.
> +///
> +// Confidence: Medium
> +// Copyright: (C) 2021 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: min, max
> +//
> +
> +
> +virtual report
> +virtual org
> +virtual context
> +virtual patch
> +
> +@rmax depends on !patch@
> +identifier func;
> +expression x, y;
> +binary operator cmp = {>, >=};
> +position p;
> +@@
> +
> +func(...)
> +{
> + <...
> +*x cmp@p y ? x : y

The rule below indicated with FIXME is supposed to deal with the
possibility of () that are unnecessary when using min and max.  It doesn't
work, because <... P ...> allow P to match 0 or more times, and thus
func@p matches every function.

A simpler solution is to just allow arbitrary () in the pattern, eg:

  (x) cmp@p (y) ? (x) : (y)

That will allow each occurrence of x and y to occur with and without
parentheses.  In the submitted  semantic patch, the () issue was only
considered in the patch case.  But it actually affects the purely matching
cases too, because () can be used at one occurrence, but not the other.

> +@script:python depends on report@
> +p << rmax.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for max()")

p is an array because it can be bound to different positions on different
control-flow paths.  Notably this occurs with <... ...>.  If there are
multiple occurrences of the pattern, there will be one match that contains
all of them.  Thus the reporting code should be:

for p0 in p:
  coccilib.report.print_report(p0, "WARNING opportunity for max()")

julia

[PATCH] media: flexcop-usb: delete unneeded return

[Julia Lawall]

No need for a return after a break;

Signed-off-by: Julia Lawall 

---
 drivers/media/usb/b2c2/flexcop-usb.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/usb/b2c2/flexcop-usb.c 
b/drivers/media/usb/b2c2/flexcop-usb.c
index e731243267e4..01d22834f4ac 100644
--- a/drivers/media/usb/b2c2/flexcop-usb.c
+++ b/drivers/media/usb/b2c2/flexcop-usb.c
@@ -195,7 +195,6 @@ static int flexcop_usb_memory_req(struct flexcop_usb 
*fc_usb,
break;
default:
return -EINVAL;
-   break;
}
for (i = 0; i < len;) {
pagechunk =

Re: [PATCH v2] coccinelle: misc: add swap script

[Julia Lawall]

On Fri, 19 Feb 2021, Denis Efremov wrote:

> Check for opencoded swap() implementation.
>
> Signed-off-by: Denis Efremov 
> ---
> Changes in v2:
>  - additional patch rule to drop excessive {}
>  - fix indentation in patch mode by anchoring ;
>
>  scripts/coccinelle/misc/swap.cocci | 101 +
>  1 file changed, 101 insertions(+)
>  create mode 100644 scripts/coccinelle/misc/swap.cocci
>
> diff --git a/scripts/coccinelle/misc/swap.cocci 
> b/scripts/coccinelle/misc/swap.cocci
> new file mode 100644
> index ..d5da9888c222
> --- /dev/null
> +++ b/scripts/coccinelle/misc/swap.cocci
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Check for opencoded swap() implementation.
> +///
> +// Confidence: High
> +// Copyright: (C) 2021 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: swap
> +//
> +
> +virtual patch
> +virtual org
> +virtual report
> +virtual context
> +
> +@r depends on !patch@
> +identifier tmp;
> +expression a, b;
> +type T;
> +position p;
> +@@
> +
> +(
> +* T tmp;
> +|
> +* T tmp = 0;
> +|
> +* T *tmp = NULL;
> +)
> +... when != tmp
> +* tmp = a;
> +* a = b;@p
> +* b = tmp;
> +... when != tmp
> +
> +@rpvar depends on patch@
> +identifier tmp;
> +expression a, b;
> +type T;
> +@@
> +
> +(
> +- T tmp;
> +|
> +- T tmp = 0;
> +|
> +- T *tmp = NULL;
> +)
> +... when != tmp
> +- tmp = a;
> +- a = b;
> +- b = tmp
> ++ swap(a, b)
> +  ;
> +... when != tmp
> +
> +
> +@rp depends on patch@
> +identifier tmp;
> +expression a, b;
> +@@
> +
> +- tmp = a;
> +- a = b;
> +- b = tmp
> ++ swap(a, b)
> +  ;

A rule like the above should also appear in the non-patch case.

> +
> +@depends on (rpvar || rp)@

This needs to be depends on patch && (rpvar || rp).  It doesn't make much
sense, because rpvar and rp both depend on patch, but at the moment that
information isn't propagate everywhere that it is needed.

thanks,
julia

> +@@
> +
> +(
> +  for (...;...;...)
> +- {
> + swap(...);
> +- }
> +|
> +  while (...)
> +- {
> + swap(...);
> +- }
> +|
> +  if (...)
> +- {
> + swap(...);
> +- }
> +)
> +
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
> --
> 2.26.2
>
>

Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const

[Julia Lawall]

On Tue, 2 Mar 2021, Bernd Petrovitsch wrote:

> Hi all!
>
> On 02/03/2021 18:42, Joe Perches wrote:
> [...]
> > - For instance: (head -10 of the git grep for file statics)
> >
> > drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 
> > 16, 8, 4, 2, 1 };
> > drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
> > drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] 
> > = {
> > drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int 
> > synth_portlist[] = { 0x2a8, 0 };
> > drivers/accessibility/speakup/speakup_decpc.c:133:static int 
> > synth_portlist[] = { 0x340, 0x350, 0x240, 0x250, 0 };
> > drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[] 
> > = {122, 89, 155, 110, 208, 240, 200, 106, 306};
> > drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[] 
> > = {86, 81, 86, 84, 81, 80, 83, 83, 73};
> > drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int 
> > synth_portlist[] = {
> > drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int 
> > synth_portlist[] = { 0x2a8, 0 };
> > drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
> >
> > For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 
> > 32, 16, 8, 4, 2, 1 };
>
> Looking at the examples: Just s/^static /static const / in the lines
> reported by the grep's above and see if it compiles (and save space)?

There is a small risk with compiling that there may be a problem for a
configuration you haven't considered.

julia

Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const

[Julia Lawall]

On Tue, 2 Mar 2021, Joe Perches wrote:

> Here is a possible opportunity to reduce data usage in the kernel.

Does it actually reduce data usage?

julia

>
> $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' 
> drivers/ | \
>   grep -v __initdata | \
>   wc -l
> 3250
>
> Meaning there are ~3000 declarations of arrays with what appears to be
> file static const content that are not marked const.
>
> So there are many static arrays that could be marked const to move the
> compiled object code from data to text minimizing the total amount of
> exposed r/w data.
>
> However, I do not know of a mechanism using coccinelle to determine
> whether or not any of these static declarations are ever modified.
>
> So it appears that each instance of these declarations might need
> manual inspection.
>
> But for arrays declared inside functions, it's much more likely that
> the static declaration without const is done with the intent to modify
> the array:
>
> (note the difference in the git grep with a leading '^\s+')
>
> $ git grep -Pn '^\s+static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' 
> drivers/ | \
>   grep -v __initdata | \
>   wc -l
> 323
>
> - For instance: (head -10 of the git grep for file statics)
>
> drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 
> 8, 4, 2, 1 };
> drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
> drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] = 
> {
> drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int 
> synth_portlist[] = { 0x2a8, 0 };
> drivers/accessibility/speakup/speakup_decpc.c:133:static int synth_portlist[] 
> = { 0x340, 0x350, 0x240, 0x250, 0 };
> drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[] = 
> {122, 89, 155, 110, 208, 240, 200, 106, 306};
> drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[] = 
> {86, 81, 86, 84, 81, 80, 83, 83, 73};
> drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int 
> synth_portlist[] = {
> drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int 
> synth_portlist[] = { 0x2a8, 0 };
> drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
>
> For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 
> 16, 8, 4, 2, 1 };
>
> masks is only used in static function say_key and should be const and
> perhaps the declaration might be better moved into that function.
>
> For drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
>
> funcvals is only used in static function spk_handle_help and should be const
> and perhaps the declaration might be better moved into that function.
>
> For drivers/accessibility/speakup/main.c:2059:static spkup_hand 
> spkup_handler[] = {
>
> spkup_handler is only used in static function do_spkup and should be const
> and perhaps the declaration might be better moved into that function.
>
> etc... for speakup
>
> For drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
>
> array ac_props is assigned as a reference in acpi_ac_add as a
> "const enum power_supply_property *" member of a struct power_supply_desc.
>
> - For instance: (head -10 of the git grep for function statics)
>
> drivers/acpi/apei/apei-base.c:781:static u8 whea_uuid_str[] = 
> "ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
> drivers/block/amiflop.c:1051: static unsigned char CRCTable1[] = {
> drivers/block/amiflop.c:1070: static unsigned char CRCTable2[] = {
> drivers/block/drbd/drbd_nl.c:872: static char units[] = { 'K', 'M', 'G', 
> 'T', 'P', 'E' };
> drivers/block/drbd/drbd_proc.c:224:   static char write_ordering_chars[] = {
> drivers/block/drbd/drbd_receiver.c:4363:  static enum drbd_conns c_tab[] 
> = {
> drivers/char/pcmcia/synclink_cs.c:3717:   static unsigned char patterns[] 
> =
> drivers/cpufreq/intel_pstate.c:1515:  static int silvermont_freq_table[] = {
> drivers/cpufreq/intel_pstate.c:1530:  static int airmont_freq_table[] = {
> drivers/dma/xgene-dma.c:360:  static u8 flyby_type[] = {
>
> Some of these could be const, but some could not.  For instance:
>
> For drivers/acpi/apei/apei-base.c:781:static u8 whea_uuid_str[] = 
> "ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
>
> whea_uuid_str is assigned as a reference in "int apei_osc_setup(void)"
> a struct acpi_osc_context where .uuid_str is not declared as const char *.
>
>
>
>
> ___
> Cocci mailing list
> co...@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

[PATCH] PM / devfreq: fix odd_ptr_err.cocci warnings (fwd)

[Julia Lawall]

Hello,

There seems to be an inconsistency, but the patch proposed by Coccinelle
does not look correct.  There should be a test on opp_table.

julia

-- Forwarded message --
Date: Mon, 1 Mar 2021 16:35:52 +0800
From: kernel test robot 
To: kbu...@lists.01.org
Cc: l...@intel.com, Julia Lawall 
Subject: [PATCH] PM / devfreq: fix odd_ptr_err.cocci warnings

CC: kbuild-...@lists.01.org
TO: Saravana Kannan 
CC: Chanwoo Choi 
CC: Sibi Sankar 
CC: MyungJoo Ham 
CC: Kyungmin Park 
CC: linux...@vger.kernel.org
CC: linux-kernel@vger.kernel.org

From: kernel test robot 

drivers/devfreq/governor_passive.c:318:7-13: inconsistent IS_ERR and PTR_ERR on 
line 319.

 PTR_ERR should access the value just tested by IS_ERR

Semantic patch information:
 There can be false positives in the patch case, where it is the call to
 IS_ERR that is wrong.

Generated by: scripts/coccinelle/tests/odd_ptr_err.cocci

Fixes: 82d4ff586ae2 ("PM / devfreq: Add cpu based scaling support to passive 
governor")
CC: Saravana Kannan 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 
---

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git 
devfreq-testing-passive-gov
head:   82d4ff586ae2fb6d89cad871949004bed3438ccb
commit: 82d4ff586ae2fb6d89cad871949004bed3438ccb [3/3] PM / devfreq: Add cpu 
based scaling support to passive governor
:: branch date: 3 hours ago
:: commit date: 3 hours ago

Please take the patch only if it's a positive warning. Thanks!

 governor_passive.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -316,7 +316,7 @@ static int cpufreq_passive_register(stru

opp_table = dev_pm_opp_get_opp_table(cpu_dev);
if (IS_ERR(devfreq->opp_table)) {
-   ret = PTR_ERR(opp_table);
+   ret = PTR_ERR(devfreq->opp_table);
goto out;
}