Re: [Outreachy kernel] [PATCH v2] staging: rtl8192u: Remove variable set but not used
Hi "Fabio,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
url:
https://github.com/0day-ci/linux/commits/Fabio-M-De-Francesco/staging-rtl8192u-Remove-variable-set-but-not-used/20210412-023753
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
1b9e18de8d43bf798622cc365f99b41f180b446f
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/0a66e6b5893f80ddaadaf4811de703afdb15bbc7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Fabio-M-De-Francesco/staging-rtl8192u-Remove-variable-set-but-not-used/20210412-023753
git checkout 0a66e6b5893f80ddaadaf4811de703afdb15bbc7
# save the attached .config to linux build tree
make W=1 W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/staging/rtl8192u/r8192U_core.c: In function 'rtl8192_hard_data_xmit':
>> drivers/staging/rtl8192u/r8192U_core.c:917:2: error: 'ret' undeclared (first
>> use in this function); did you mean 'net'?
917 | ret = rtl8192_tx(dev, skb);
| ^~~
| net
drivers/staging/rtl8192u/r8192U_core.c:917:2: note: each undeclared
identifier is reported only once for each function it appears in
vim +917 drivers/staging/rtl8192u/r8192U_core.c
8fc8598e61f6f3 Jerry Chuang 2009-11-03 897
8fc8598e61f6f3 Jerry Chuang 2009-11-03 898 /* this function TX data
frames when the ieee80211 stack requires this.
8fc8598e61f6f3 Jerry Chuang 2009-11-03 899 * It checks also if we need
to stop the ieee tx queue, eventually do it
8fc8598e61f6f3 Jerry Chuang 2009-11-03 900 */
069b3162590896 Raphaël Beamonte 2015-09-20 901 static void
rtl8192_hard_data_xmit(struct sk_buff *skb, struct net_device *dev,
069b3162590896 Raphaël Beamonte 2015-09-20 902
int rate)
8fc8598e61f6f3 Jerry Chuang 2009-11-03 903 {
8fc8598e61f6f3 Jerry Chuang 2009-11-03 904 struct r8192_priv *priv
= (struct r8192_priv *)ieee80211_priv(dev);
8fc8598e61f6f3 Jerry Chuang 2009-11-03 905 unsigned long flags;
20f896c4dbb48f simran singhal 2017-02-12 906 struct cb_desc
*tcb_desc = (struct cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
8fc8598e61f6f3 Jerry Chuang 2009-11-03 907 u8 queue_index =
tcb_desc->queue_index;
8fc8598e61f6f3 Jerry Chuang 2009-11-03 908
8fc8598e61f6f3 Jerry Chuang 2009-11-03 909 /* shall not be
referred by command packet */
4a8d1135548baf Xenia Ragiadakou 2013-06-09 910
RTL8192U_ASSERT(queue_index != TXCMD_QUEUE);
8fc8598e61f6f3 Jerry Chuang 2009-11-03 911
8fc8598e61f6f3 Jerry Chuang 2009-11-03 912
spin_lock_irqsave(&priv->tx_lock, flags);
8fc8598e61f6f3 Jerry Chuang 2009-11-03 913
c3f463484bdd0a Ben Hutchings2016-04-21 914 *(struct net_device
**)(skb->cb) = dev;
8fc8598e61f6f3 Jerry Chuang 2009-11-03 915
tcb_desc->bTxEnableFwCalcDur = 1;
8fc8598e61f6f3 Jerry Chuang 2009-11-03 916 skb_push(skb,
priv->ieee80211->tx_headroom);
8fc8598e61f6f3 Jerry Chuang 2009-11-03 @917 ret = rtl8192_tx(dev,
skb);
8fc8598e61f6f3 Jerry Chuang 2009-11-03 918
8fc8598e61f6f3 Jerry Chuang 2009-11-03 919
spin_unlock_irqrestore(&priv->tx_lock, flags);
8fc8598e61f6f3 Jerry Chuang 2009-11-03 920 }
8fc8598e61f6f3 Jerry Chuang 2009-11-03 921
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [Outreachy kernel] [PATCH] staging: rtl8192u: Remove function
Hi "Fabio,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
url:
https://github.com/0day-ci/linux/commits/Fabio-M-De-Francesco/staging-rtl8192u-Remove-function/20210412-024938
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
1b9e18de8d43bf798622cc365f99b41f180b446f
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/499674dec8e01774889806d098bf9a12731930ee
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Fabio-M-De-Francesco/staging-rtl8192u-Remove-function/20210412-024938
git checkout 499674dec8e01774889806d098bf9a12731930ee
# save the attached .config to linux build tree
make W=1 W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/staging/rtl8192u/r819xU_cmdpkt.c: In function
'cmpk_message_handle_rx':
>> drivers/staging/rtl8192u/r819xU_cmdpkt.c:477:4: error: implicit declaration
>> of function 'cmpk_handle_query_config_rx'
>> [-Werror=implicit-function-declaration]
477 |cmpk_handle_query_config_rx(dev, pcmd_buff);
|^~~
cc1: some warnings being treated as errors
vim +/cmpk_handle_query_config_rx +477 drivers/staging/rtl8192u/r819xU_cmdpkt.c
8fc8598e61f6f3 Jerry Chuang 2009-11-03 409
8fc8598e61f6f3 Jerry Chuang 2009-11-03 410
/*-
8fc8598e61f6f3 Jerry Chuang 2009-11-03 411 * Function:
cmpk_message_handle_rx()
8fc8598e61f6f3 Jerry Chuang 2009-11-03 412 *
8fc8598e61f6f3 Jerry Chuang 2009-11-03 413 * Overview:In the
function, we will capture different RX command packet
8fc8598e61f6f3 Jerry Chuang 2009-11-03 414 *info. Every RX
command packet element has different message
8fc8598e61f6f3 Jerry Chuang 2009-11-03 415 *length and
meaning in content. We only support three type of RX
8fc8598e61f6f3 Jerry Chuang 2009-11-03 416 *command packet
now. Please refer to document
8fc8598e61f6f3 Jerry Chuang 2009-11-03 417 *
ws-06-0063-rtl8190-command-packet-specification.
8fc8598e61f6f3 Jerry Chuang 2009-11-03 418 *
8fc8598e61f6f3 Jerry Chuang 2009-11-03 419 * Input: NONE
8fc8598e61f6f3 Jerry Chuang 2009-11-03 420 *
8fc8598e61f6f3 Jerry Chuang 2009-11-03 421 * Output: NONE
8fc8598e61f6f3 Jerry Chuang 2009-11-03 422 *
8fc8598e61f6f3 Jerry Chuang 2009-11-03 423 * Return: NONE
8fc8598e61f6f3 Jerry Chuang 2009-11-03 424 *
8fc8598e61f6f3 Jerry Chuang 2009-11-03 425 * Revised History:
8fc8598e61f6f3 Jerry Chuang 2009-11-03 426 * When Who
Remark
8fc8598e61f6f3 Jerry Chuang 2009-11-03 427 * 05/06/2008
amy Create Version 0 porting from windows code.
8fc8598e61f6f3 Jerry Chuang 2009-11-03 428 *
70cd55d6755ee8 Derek Robson 2017-02-16 429
*---
70cd55d6755ee8 Derek Robson 2017-02-16 430 */
a115ee4175c3eb Teodora Baluta2013-10-16 431 u32
cmpk_message_handle_rx(struct net_device *dev,
8fc8598e61f6f3 Jerry Chuang 2009-11-03 432
struct ieee80211_rx_stats *pstats)
8fc8598e61f6f3 Jerry Chuang 2009-11-03 433 {
8fc8598e61f6f3 Jerry Chuang 2009-11-03 434int
total_length;
8fc8598e61f6f3 Jerry Chuang 2009-11-03 435u8
cmd_length, exe_cnt = 0;
8fc8598e61f6f3 Jerry Chuang 2009-11-03 436u8
element_id;
8fc8598e61f6f3 Jerry Chuang 2009-11-03 437u8
*pcmd_buff;
8fc8598e61f6f3 Jerry Chuang 2009-11-03 438
dc109dc597d7f4 simran singhal2017-03-04 439/* 0. Check inpt
arguments. It is a command queue message or
70cd55d6755ee8 Derek Robson 2017-02-16 440 * pointer is null.
70cd55d6755ee8 Derek Robson 2017-02-16 441 */
d6628e8cbe2047 Michael Straube 2020-09-19 442if (!pstats)
8fc8598e61f6f3 Jerry Chuang 2009-11-03 443return 0;
/* This is not a command packet. */
8fc8598e61f6f3 Jerry Chuang 2009-11-03 444
8fc8598e61f6f3 Jerry Chuang 2009-11-03 445/* 1. Read received
command packet message length from RFD. */
8fc8598e61f6f3 Jerry Chuang 2009-11-03 446total_length =
pstats->Length;
8fc8598e61f6f3 Jerry Chuang 2009-11-03 447
8fc8598e61f6f3 Jerry Chuang 2009-11-03 448/* 2. Read virtual
address from RFD. */
8fc8598e61f6f3 Jerry Chuang 2009-11-03
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Fri, Apr 16, 2021 at 11:37:28AM +0300, Sakari Ailus wrote: > Hi Dan, > > On Fri, Apr 16, 2021 at 08:49:41AM +0300, Dan Carpenter wrote: > > On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote: > > > On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote: > > > > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > > > > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > > > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro > > > > > > > wrote: > > > > > > > > -const struct atomisp_format_bridge > > > > > > > > *get_atomisp_format_bridge_from_mbus( > > > > > > > > -u32 mbus_code); > > > > > > > > +const struct atomisp_format_bridge* > > > > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > > > > > > > > > No, this does not match coding style. Probably best to break the > > > > > > > 80-column guideline in this instance. Best would be to have a > > > > > > > function > > > > > > > > > > > > Having the return type on the previous line is perfectly fine. > > > > > > There should > > > > > > be a space before the asterisk though. > > > > > > > > > > No, it's not. Linus has ranted about that before. > > > > > > > > Found it. > > > > https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/ > > > > > > Two decades ago, really? > > > > > > This is simply one of the practical means how you split long function > > > declarations and avoid overly long lines. Not my favourite though, but > > > still better than those long lines. > > > > I've always thought we allow either style, but it has to be done > > consistently within the file. I was pretty sure that was policy but > > it's another thing that goes back decades so I don't have a reference. > > It shouldn't be about breaking up long lines. > > > > > > > > My personal preference would be to wrap at the opening parenthesis and > > > indent by just a tab, but I know many people who disagree with that... > > > > 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. The prefered style is still aligning with the parentheses but if you have to choose between a warning about going over the limit or a warning about aligning then probably it's fine to not align. I can't find an example, but I'm pretty sure we've been rejecting patches that align the parameters but now go over the 80/100 char limit. regards, dan carpenter
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
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
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. -- Sakari Ailus
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
> > 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] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
Hi Dan, On Fri, Apr 16, 2021 at 08:49:41AM +0300, Dan Carpenter wrote: > On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote: > > On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote: > > > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > > > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro > > > > > > wrote: > > > > > > > -const struct atomisp_format_bridge > > > > > > > *get_atomisp_format_bridge_from_mbus( > > > > > > > -u32 mbus_code); > > > > > > > +const struct atomisp_format_bridge* > > > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > > > > > > > No, this does not match coding style. Probably best to break the > > > > > > 80-column guideline in this instance. Best would be to have a > > > > > > function > > > > > > > > > > Having the return type on the previous line is perfectly fine. There > > > > > should > > > > > be a space before the asterisk though. > > > > > > > > No, it's not. Linus has ranted about that before. > > > > > > Found it. > > > https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/ > > > > Two decades ago, really? > > > > This is simply one of the practical means how you split long function > > declarations and avoid overly long lines. Not my favourite though, but > > still better than those long lines. > > I've always thought we allow either style, but it has to be done > consistently within the file. I was pretty sure that was policy but > it's another thing that goes back decades so I don't have a reference. > It shouldn't be about breaking up long lines. > > > > > My personal preference would be to wrap at the opening parenthesis and > > indent by just a tab, but I know many people who disagree with that... > > 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. -- Kind regards, Sakari Ailus
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote: > On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote: > > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro > > > > > wrote: > > > > > > -const struct atomisp_format_bridge > > > > > > *get_atomisp_format_bridge_from_mbus( > > > > > > -u32 mbus_code); > > > > > > +const struct atomisp_format_bridge* > > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > > > > > No, this does not match coding style. Probably best to break the > > > > > 80-column guideline in this instance. Best would be to have a > > > > > function > > > > > > > > Having the return type on the previous line is perfectly fine. There > > > > should > > > > be a space before the asterisk though. > > > > > > No, it's not. Linus has ranted about that before. > > > > Found it. > > https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/ > > Two decades ago, really? > > This is simply one of the practical means how you split long function > declarations and avoid overly long lines. Not my favourite though, but > still better than those long lines. I've always thought we allow either style, but it has to be done consistently within the file. I was pretty sure that was policy but it's another thing that goes back decades so I don't have a reference. It shouldn't be about breaking up long lines. > > My personal preference would be to wrap at the opening parenthesis and > indent by just a tab, but I know many people who disagree with that... 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. regards, dan carpenter
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote: > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > > > > > -const struct atomisp_format_bridge > > > > > *get_atomisp_format_bridge_from_mbus( > > > > > -u32 mbus_code); > > > > > +const struct atomisp_format_bridge* > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > > > No, this does not match coding style. Probably best to break the > > > > 80-column guideline in this instance. Best would be to have a function > > > > > > Having the return type on the previous line is perfectly fine. There > > > should > > > be a space before the asterisk though. > > > > No, it's not. Linus has ranted about that before. > > Found it. > https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/ Two decades ago, really? This is simply one of the practical means how you split long function declarations and avoid overly long lines. Not my favourite though, but still better than those long lines. My personal preference would be to wrap at the opening parenthesis and indent by just a tab, but I know many people who disagree with that... -- Sakari Ailus
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > > > > -const struct atomisp_format_bridge > > > > *get_atomisp_format_bridge_from_mbus( > > > > -u32 mbus_code); > > > > +const struct atomisp_format_bridge* > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > No, this does not match coding style. Probably best to break the > > > 80-column guideline in this instance. Best would be to have a function > > > > Having the return type on the previous line is perfectly fine. There should > > be a space before the asterisk though. > > No, it's not. Linus has ranted about that before. Found it. https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > > > -const struct atomisp_format_bridge *get_atomisp_format_bridge_from_mbus( > > > -u32 mbus_code); > > > +const struct atomisp_format_bridge* > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > No, this does not match coding style. Probably best to break the > > 80-column guideline in this instance. Best would be to have a function > > Having the return type on the previous line is perfectly fine. There should > be a space before the asterisk though. No, it's not. Linus has ranted about that before.
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > > -const struct atomisp_format_bridge *get_atomisp_format_bridge_from_mbus( > > -u32 mbus_code); > > +const struct atomisp_format_bridge* > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > No, this does not match coding style. Probably best to break the > 80-column guideline in this instance. Best would be to have a function Having the return type on the previous line is perfectly fine. There should be a space before the asterisk though. -- Sakari Ailus
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
Em qui, 2021-04-15 às 18:14 +0100, Matthew Wilcox escreveu: > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro > wrote: > > -const struct atomisp_format_bridge > > *get_atomisp_format_bridge_from_mbus( > > - u32 mbus_code); > > +const struct atomisp_format_bridge* > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > No, this does not match coding style. Probably best to break the > 80-column guideline in this instance. Best would be to have a > function > and/or struct name that isn't so ridiculously long, but that would > require some in-depth thinking. > I left the type of function and its name with the parameters in different lines, following up some examples of other files, such as atomisp_acc.c. But I didn't pay attention and left the pointer with the function name instead of left it with the type of the function in v1, so Hans suggested it to a v2, as I did. What should I do in this case? Thank you in advance, Aline > > -void atomisp_apply_css_parameters( > > - struct atomisp_sub_device *asd, > > - struct atomisp_css_params *css_param); > > +void atomisp_apply_css_parameters(struct atomisp_sub_device *asd, > > + struct atomisp_css_params > > *css_param); > > + > > Good. >
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > -const struct atomisp_format_bridge *get_atomisp_format_bridge_from_mbus( > -u32 mbus_code); > +const struct atomisp_format_bridge* > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); No, this does not match coding style. Probably best to break the 80-column guideline in this instance. Best would be to have a function and/or struct name that isn't so ridiculously long, but that would require some in-depth thinking. > -void atomisp_apply_css_parameters( > -struct atomisp_sub_device *asd, > -struct atomisp_css_params *css_param); > +void atomisp_apply_css_parameters(struct atomisp_sub_device *asd, > + struct atomisp_css_params *css_param); > + Good.
Re: [Outreachy kernel] Re: [PATCH v4 2/2] staging: media: zoran: add BIT() macro and align code
> > > +#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 kernel] [PATCH v4] staging: rtl8723bs: Remove led_blink_hdl() and everything related
On Thursday, April 15, 2021 9:28:46 AM CEST Greg Kroah-Hartman wrote: > On Thu, Apr 15, 2021 at 09:17:31AM +0200, Fabio M. De Francesco wrote: > > Removed useless led_blink_hdl() prototype and definition. > > Removed struct LedBlink_param. Removed LedBlink entries in > > rtw_cmd_callback[] and in wlancmds[]. Everything related to LedBlink is > > not anymore needed. Index of slots changed in arrays comments to > > reflect > > current positions. > > > > Reported-by: Julia Lawall > > Reported-by: Fabio Aiuto > > Reported-by: Greg Kroah-Hartman > > Suggested-by: Matthew Wilcox > > Suggested-by: Dan Carpenter > > Signed-off-by: Fabio M. De Francesco > > --- > > > > Changes from v3: Merged the series into one single patch for avoiding > > unnecessary intermediate stages. > > Much better, thanks for sticking with this. > I think it was worth doing this job well and getting it done, despite the difficulties I had. I really appreciate your message. Thanks so much, Fabio > > Now queued up, and I think > this is going to be the last patch I take for 5.13-rc1. The rest I'll > store up for the next kernel release after that... > > thanks, > > greg k-h
Re: [Outreachy kernel] [PATCH v4] staging: rtl8723bs: Remove led_blink_hdl() and everything related
On Thu, Apr 15, 2021 at 09:17:31AM +0200, Fabio M. De Francesco wrote: > Removed useless led_blink_hdl() prototype and definition. > Removed struct LedBlink_param. Removed LedBlink entries in > rtw_cmd_callback[] and in wlancmds[]. Everything related to LedBlink is > not anymore needed. Index of slots changed in arrays comments to reflect > current positions. > > Reported-by: Julia Lawall > Reported-by: Fabio Aiuto > Reported-by: Greg Kroah-Hartman > Suggested-by: Matthew Wilcox > Suggested-by: Dan Carpenter > Signed-off-by: Fabio M. De Francesco > --- > > Changes from v3: Merged the series into one single patch for avoiding > unnecessary intermediate stages. Much better, thanks for sticking with this. Now queued up, and I think this is going to be the last patch I take for 5.13-rc1. The rest I'll store up for the next kernel release after that... thanks, greg k-h
[Outreachy kernel] [PATCH v4] staging: rtl8723bs: Remove led_blink_hdl() and everything related
Removed useless led_blink_hdl() prototype and definition.
Removed struct LedBlink_param. Removed LedBlink entries in
rtw_cmd_callback[] and in wlancmds[]. Everything related to LedBlink is
not anymore needed. Index of slots changed in arrays comments to reflect
current positions.
Reported-by: Julia Lawall
Reported-by: Fabio Aiuto
Reported-by: Greg Kroah-Hartman
Suggested-by: Matthew Wilcox
Suggested-by: Dan Carpenter
Signed-off-by: Fabio M. De Francesco
---
Changes from v3: Merged the series into one single patch for avoiding
unnecessary intermediate stages.
Changes from v2: Made a series and added another patch (2/2).
Changes from v1: Corrected a bad solution to this issue that made use of
an unnecessary dummy function.
drivers/staging/rtl8723bs/core/rtw_cmd.c | 16 +++-
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
drivers/staging/rtl8723bs/include/rtw_cmd.h | 14 --
drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
4 files changed, 11 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 0297fbad7bce..d834a82aaf55 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -78,13 +78,12 @@ static struct _cmd_callback rtw_cmd_callback[] = {
{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(_SetChannelSwitch), NULL},/*60*/
+ {GEN_CMD_CODE(_TDLS), NULL},/*61*/
+ {GEN_CMD_CODE(_ChkBMCSleepq), NULL}, /*62*/
- {GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
+ {GEN_CMD_CODE(_RunInThreadCMD), NULL},/*63*/
};
static struct cmd_hdl wlancmds[] = {
@@ -150,11 +149,10 @@ 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*/
- 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/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 873d3792ac8e..963ea80083c8 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_cmd.h
b/drivers/staging/rtl8723bs/include/rtw_cmd.h
index 517ae3b51386..28d2d2732374 100644
--- a/drivers/staging/rtl8723bs/include/rtw_cmd.h
+++ b/drivers/staging/rtl8723bs/include/rtw_cmd.h
@@ -537,11 +537,6 @@ struct SetChannelPlan_param {
u8 channel_plan;
};
-/*H2C Handler index: 60 */
-struct LedBlink_param {
- void *pLed;
-};
-
/*H2C Handler index: 61 */
struct SetChannelSwitch_param {
u8 new_ch_no;
@@ -709,13 +704,12 @@ enum {
GEN_CMD_CODE(_Set_H2C_MSG), /*58*/
GEN_CMD_CODE(_SetChannelPlan), /*59*/
- GEN_CMD_CODE(_LedBlink), /*60*/
- GEN_CMD_CODE(_SetChannelSwitch), /*61*/
- GEN_CMD_CODE(_TDLS), /*62*/
- GEN_CMD_CODE(_ChkBMCSleepq), /*63*/
+ GEN_CMD_CODE(_SetChannelSwitch), /*60*/
+ GEN_CMD_CODE(_TDLS), /*61*/
+ GEN_CMD_CODE(_ChkBMCSleepq), /*62*/
- GEN_CMD_CODE(_RunInThreadCMD), /*64*/
+ GEN_CMD_CODE(_RunInThreadCMD), /*63*/
MAX_H2CCMD
};
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 se
Re: [Outreachy patch] [PATCH v3 1/2] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 09:27:49PM +0200, 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.
>
> Reported-by: Julia Lawall
> Suggested-by: Matthew Wilcox
> Suggested-by: Dan Carpenter
> Signed-off-by: Fabio M. De Francesco
> ---
>
> Changes from v2: no changes.
> Changes from v1: Corrected a bad solution to this issue that made use of
> an unnecessary dummy function.
>
> drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> 3 files changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..f82dbd4f4c3d 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -150,7 +150,7 @@ 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*/
> + GEN_MLME_EXT_HANDLER(0, NULL) /*60*/
Do not do this "intermediate stage" type thing like this please. This
should be one simple patch. Again, look in the kernel archives for an
example of how to do this.
thanks,
greg k-h
Re: [Outreachy kernel] [PATCH v3 2/2] staging: rtl8723bs: Remove everything related with LedBlink
On Wed, Apr 14, 2021 at 09:27:50PM +0200, 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.
>
> 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},
These blank lines are there for a reason, please do not remove them.
thanks,
gre gk-h
Re: [Outreachy patch] [PATCH v3 1/2] staging: rtl8723bs: Remove useless led_blink_hdl()
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
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), &rtw_survey_cmd_callback}, /*18*/
> {GEN_CMD_CODE(_SetAuth), NULL},
> -
> {GEN_CMD_CODE(_SetKey), NULL}, /*20*/
> {GEN_CMD_CODE(_SetStaKey), &rtw_setstaKey_cmdrsp_callback},
> {GEN_CMD_CODE(_SetAssocSta), &rtw_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
> b/drivers/staging/rtl872
[Outreachy kernel] [PATCH v3 2/2] staging: rtl8723bs: Remove everything related with LedBlink
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.
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), &rtw_survey_cmd_callback}, /*18*/
{GEN_CMD_CODE(_SetAuth), NULL},
-
{GEN_CMD_CODE(_SetKey), NULL}, /*20*/
{GEN_CMD_CODE(_SetStaKey), &rtw_setstaKey_cmdrsp_callback},
{GEN_CMD_CODE(_SetAssocSta), &rtw_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
b/drivers/staging/rtl8723bs/include/rtw_cmd.h
index 517ae3b51386..28d2d2732374 100644
--- a/drivers/staging/rtl8723bs/include/rtw_cmd.h
+++ b/drivers/staging/rtl8723bs/include/rtw_cmd.h
@@ -537,11 +537,6 @@ struct SetChannelPlan_param {
u8 channel_plan;
};
-/*H2C Handler index: 60 */
-struct LedBli
[Outreachy patch] [PATCH v3 1/2] staging: rtl8723bs: Remove useless led_blink_hdl()
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.
Reported-by: Julia Lawall
Suggested-by: Matthew Wilcox
Suggested-by: Dan Carpenter
Signed-off-by: Fabio M. De Francesco
---
Changes from v2: no changes.
Changes from v1: Corrected a bad solution to this issue that made use of
an unnecessary dummy function.
drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
3 files changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 0297fbad7bce..f82dbd4f4c3d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -150,7 +150,7 @@ 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*/
+ 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*/
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 873d3792ac8e..963ea80083c8 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
[Outreachy kernel] [PATCH v3 0/2] Remove led_blink_hdl and other related symbols
Removed useless led_blink_hdl() prototype and definition. 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. Fabio M. De Francesco (2): staging: rtl8723bs: Remove useless led_blink_hdl() staging: rtl8723bs: Remove everything related with LedBlink drivers/staging/rtl8723bs/core/rtw_cmd.c | 27 +-- drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 9 --- drivers/staging/rtl8723bs/include/rtw_cmd.h | 14 +++--- .../staging/rtl8723bs/include/rtw_mlme_ext.h | 1 - 4 files changed, 11 insertions(+), 40 deletions(-) -- 2.31.1
Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wednesday, April 14, 2021 8:05:59 PM CEST Fabio M. De Francesco wrote:
> On Wednesday, April 14, 2021 7:57:03 PM CEST Greg Kroah-Hartman wrote:
> > On Wed, Apr 14, 2021 at 08:48:09PM +0300, Dan Carpenter wrote:
> > > On Wed, Apr 14, 2021 at 07:00:41PM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Apr 14, 2021 at 06:26:14PM +0200, 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.
> > > > >
> > > > > Reported-by: Julia Lawall
> > > > > Suggested-by: Dan Carpenter
> > > > > Signed-off-by: Fabio M. De Francesco
> > > > > ---
> > > > >
> > > > > Changes since v1: Corrected a bad solution to this issue that
> > > > > made
> > > > > use of an unnecessary dummy function.
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> > > > > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> > > > > drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > > 3 files changed, 1 insertion(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > 0297fbad7bce..f82dbd4f4c3d 100644
> > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > @@ -150,7 +150,7 @@ 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*/ + GEN_MLME_EXT_HANDLER(0, NULL) /
*60*/
> > > >
> > > > Better, but you really do not need to keep this here, right?
> > > > Remove
> > > > the
> > > > "led blink command" entirely, you didn't do that here.
> > >
> > > No, this is right. We have to put a NULL function pointer in the
> > > array
> > > or the indexing will be off. But Fabio is correct that the struct
> > > type should be removed.
> >
> > The indexing can be off, just remove the other place where the
> > "command"
> > is in the index and all is good as rebuilding will fix it. These are
> > not external "values" we have to keep stable.
> >
> > This has been done for other drivers exactly like this, there are loads
> > of "odd" commands in there that should not be.
> >
> > thanks,
> >
> > greg k-h
>
> I'm not sure if this task is so close related to deserve a v3 or if I
> should make a new v1 patch with a different "Subject".
>
> Thanks,
>
> Fabio
I'll make a v3 series, submitting this patch again (as 1/2) and adding the
above-mentioned changes in another one (as 2/2).
Fabio
Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wednesday, April 14, 2021 7:57:03 PM CEST Greg Kroah-Hartman wrote:
> On Wed, Apr 14, 2021 at 08:48:09PM +0300, Dan Carpenter wrote:
> > On Wed, Apr 14, 2021 at 07:00:41PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Apr 14, 2021 at 06:26:14PM +0200, 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.
> > > >
> > > > Reported-by: Julia Lawall
> > > > Suggested-by: Dan Carpenter
> > > > Signed-off-by: Fabio M. De Francesco
> > > > ---
> > > >
> > > > Changes since v1: Corrected a bad solution to this issue that made
> > > > use of an unnecessary dummy function.
> > > >
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> > > > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> > > > drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > 3 files changed, 1 insertion(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > 0297fbad7bce..f82dbd4f4c3d 100644
> > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > @@ -150,7 +150,7 @@ 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*/ + GEN_MLME_EXT_HANDLER(0, NULL) /*60*/
> > >
> > > Better, but you really do not need to keep this here, right? Remove
> > > the
> > > "led blink command" entirely, you didn't do that here.
> >
> > No, this is right. We have to put a NULL function pointer in the array
> > or the indexing will be off. But Fabio is correct that the struct
> > type should be removed.
>
> The indexing can be off, just remove the other place where the "command"
> is in the index and all is good as rebuilding will fix it. These are
> not external "values" we have to keep stable.
>
> This has been done for other drivers exactly like this, there are loads
> of "odd" commands in there that should not be.
>
> thanks,
>
> greg k-h
I'm not sure if this task is so close related to deserve a v3 or if I
should make a new v1 patch with a different "Subject".
Thanks,
Fabio
Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 08:48:09PM +0300, Dan Carpenter wrote:
> On Wed, Apr 14, 2021 at 07:00:41PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Apr 14, 2021 at 06:26:14PM +0200, 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.
> > >
> > > Reported-by: Julia Lawall
> > > Suggested-by: Dan Carpenter
> > > Signed-off-by: Fabio M. De Francesco
> > > ---
> > >
> > > Changes since v1: Corrected a bad solution to this issue that made use of
> > > an unnecessary dummy function.
> > >
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> > > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> > > drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > 3 files changed, 1 insertion(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > index 0297fbad7bce..f82dbd4f4c3d 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > @@ -150,7 +150,7 @@ 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*/
> > > + GEN_MLME_EXT_HANDLER(0, NULL) /*60*/
> >
> > Better, but you really do not need to keep this here, right? Remove the
> > "led blink command" entirely, you didn't do that here.
>
> No, this is right. We have to put a NULL function pointer in the array
> or the indexing will be off. But Fabio is correct that the struct
> type should be removed.
The indexing can be off, just remove the other place where the "command"
is in the index and all is good as rebuilding will fix it. These are
not external "values" we have to keep stable.
This has been done for other drivers exactly like this, there are loads
of "odd" commands in there that should not be.
thanks,
greg k-h
Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 07:00:41PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 14, 2021 at 06:26:14PM +0200, 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.
> >
> > Reported-by: Julia Lawall
> > Suggested-by: Dan Carpenter
> > Signed-off-by: Fabio M. De Francesco
> > ---
> >
> > Changes since v1: Corrected a bad solution to this issue that made use of
> > an unnecessary dummy function.
> >
> > drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> > drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > 3 files changed, 1 insertion(+), 11 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > index 0297fbad7bce..f82dbd4f4c3d 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > @@ -150,7 +150,7 @@ 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*/
> > + GEN_MLME_EXT_HANDLER(0, NULL) /*60*/
>
> Better, but you really do not need to keep this here, right? Remove the
> "led blink command" entirely, you didn't do that here.
No, this is right. We have to put a NULL function pointer in the array
or the indexing will be off. But Fabio is correct that the struct
type should be removed.
regards,
dan carpenter
Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 06:26:14PM +0200, 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.
>
> Reported-by: Julia Lawall
> Suggested-by: Dan Carpenter
> Signed-off-by: Fabio M. De Francesco
> ---
>
> Changes since v1: Corrected a bad solution to this issue that made use of
> an unnecessary dummy function.
>
> drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> 3 files changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..f82dbd4f4c3d 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -150,7 +150,7 @@ 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*/
> + GEN_MLME_EXT_HANDLER(0, NULL) /*60*/
Better, but you really do not need to keep this here, right? Remove the
"led blink command" entirely, you didn't do that here.
thanks,
greg k-h
Re: [Outreachy kernel] [PATCH v2] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 06:26:14PM +0200, 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.
>
> Reported-by: Julia Lawall
> Suggested-by: Dan Carpenter
> Signed-off-by: Fabio M. De Francesco
> ---
>
> Changes since v1: Corrected a bad solution to this issue that made use of
> an unnecessary dummy function.
>
> drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> 3 files changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..f82dbd4f4c3d 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -150,7 +150,7 @@ 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*/
> + GEN_MLME_EXT_HANDLER(0, NULL) /*60*/
I'd remove LedBlink_param struct as well, now it's unused
>
> 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 873d3792ac8e..963ea80083c8 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
>
>
thank you,
fabio
[Outreachy kernel] [PATCH v2] staging: rtl8723bs: Remove useless led_blink_hdl()
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.
Reported-by: Julia Lawall
Suggested-by: Dan Carpenter
Signed-off-by: Fabio M. De Francesco
---
Changes since v1: Corrected a bad solution to this issue that made use of
an unnecessary dummy function.
drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
3 files changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 0297fbad7bce..f82dbd4f4c3d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -150,7 +150,7 @@ 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*/
+ 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*/
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 873d3792ac8e..963ea80083c8 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
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wednesday, April 14, 2021 3:24:14 PM CEST Dan Carpenter wrote:
> On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> > Removed the led_blink_hdl() function (declaration and definition).
> > Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> > core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> > macro to make use of dummy_function().
> >
> > Reported-by: Julia Lawall
> > Suggested-by: Dan Carpenter
> > Signed-off-by: Fabio M. De Francesco
> > ---
> >
> > drivers/staging/rtl8723bs/core/rtw_cmd.c | 4 +++-
> > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> > drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++-
> > 3 files changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > 0297fbad7bce..7b6102a2bb2c 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > @@ -87,6 +87,8 @@ static struct _cmd_callback rtw_cmd_callback[] = {
> >
> > {GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
> >
> > };
> >
> > +u8 dummy_functioni(struct adapter *var0, u8 *var1) { return 0; }
> > +
> >
> > static struct cmd_hdl wlancmds[] = {
> >
> > GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
> > GEN_DRV_CMD_HANDLER(0, NULL)
> >
> > @@ -150,7 +152,7 @@ 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*/ +GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> > dummy_function) /*60*/
> No, no. Don't create a dummy function. Do it like so:
>
> GEN_DRV_CMD_HANDLER(0, NULL) /* 60 */
>
> regards,
> dan carpenter
>
I'm replying late because I didn't want to blindly use that solution; I
mean that I wanted to understand why I can simply put 0 and NULL into that
macro. I had seen it made in other lines that initialize wlancmds[]
elements, but I wasn't sure if it could work for the specific slot where
the pointer to led_blinck_hdl was supposed to be placed.
Now I think that it is why, in this case, cmd_hdl would be set to NULL by
the call to wlancmds[pcmd->cmdcode].h2cfuns and the cmd_hdl() function
wouldn't be called because cmd_hdl is tested within an "if" statement.
Therefore a simple GEN_DRV_CMD_HANDLER(0, NULL) at slot number would be the
simplest and most obvious solution.
Is the above argument sound?
Thanks for your kind help,
Fabio
Re: [Outreachy kernel] Re: [PATCH 1/2] staging: media: atomisp: pci: Correct identation in block of conditional statements in file atomisp_v4l2.c
Em qua, 2021-04-14 às 17:45 +0300, Dan Carpenter escreveu:
> On Wed, Apr 14, 2021 at 04:39:20PM +0200, Julia Lawall wrote:
> >
> >
> > 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 <
> > > > alinesantanacorde...@gmail.com>
> > > > ---
> > > > 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(&isp-
> > > > >csi2_port[i]);
> > > >
> > > > list_for_each_entry_safe(sd, next, &isp-
> > > > >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, &isp-
> > > >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.
> >
>
> Huh... You're right. Never mind then.
>
> regards,
> dan carpenter
>
Ok then :)
Thank you all,
Aline
Re: [Outreachy kernel] Re: [PATCH 1/2] staging: media: atomisp: pci: Correct identation in block of conditional statements in file atomisp_v4l2.c
On Wed, Apr 14, 2021 at 04:39:20PM +0200, Julia Lawall wrote:
>
>
> 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(&isp->csi2_port[i]);
> > >
> > > list_for_each_entry_safe(sd, next, &isp->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, &isp->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.
>
Huh... You're right. Never mind then.
regards,
dan carpenter
Re: [Outreachy kernel] Re: [PATCH 1/2] staging: media: atomisp: pci: Correct identation in block of conditional statements in file atomisp_v4l2.c
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(&isp->csi2_port[i]);
> >
> > list_for_each_entry_safe(sd, next, &isp->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, &isp->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] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wednesday, April 14, 2021 2:18:13 PM CEST Greg Kroah-Hartman wrote:
> On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> > Removed the led_blink_hdl() function (declaration and definition).
> > Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> > core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> > macro to make use of dummy_function().
>
> No no no.
>
> If you want to remove is function declaration and use, then do it
> properly.
>
> The code is crazy, I agree, but it should not be difficult to just
> remove this correctly instead of papering over this mess.
>
> Also note that no one actually calls this function if you look at the
> logic here.
>
> It might take some good knowledge of C to unwind this crud,
> but once done, you should be able to "prove" it's not called
>
Proving that no one actually calls it it's beyond my
current knowledge of programming with C.
Matthew W., who is for sure more experienced than I am ,
wrote that that function pointer in the array is used somewhere else.
Copied and pasted here from his message:
"Here's where the driver calls that function:
$ git grep wlancmds drivers/staging/rtl8723bs/
drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[] = {
drivers/staging/rtl8723bs/core/rtw_cmd.c: if (pcmd->cmdcode <
ARRAY_SIZE(wlancmds)) {
drivers/staging/rtl8723bs/core/rtw_cmd.c: cmd_hdl =
wlancmds[pcmd->cmdcode].h2cfuns;
>
> and how to
> remove it correctly.
>
I think that doing it correctly depends on the "prove" which you requested.
Doesn't it?
>
> And no, I'm not going to say how to do it, that's an exercise best left
> for the reader.
>
It sounds perfectly reasonable and I agree in full.
>
> But I will hint that this was done in the past, in
> 2014, in another driver in the tree with a codebase much like this one,
> so it shouldn't be hard to find an example of it. Only took me a few
> minutes...
>
I'm sure it took you only a few minutes. If this can be accomplished by using
grep
on git log output I need some time to read this command manual again. I suppose
that the search should be made by combining "remove", "function",
"drivers/staging",
and "2014". At the moment I don't know how to do that.
Notwithstanding I have said all that you read above, you can be sure that I
won't give
up so easily even if it will take days :)
>
> good luck!
>
Thanks,
Fabio
>
> greg k-h
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> Removed the led_blink_hdl() function (declaration and definition).
> Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> macro to make use of dummy_function().
>
> Reported-by: Julia Lawall
> Suggested-by: Dan Carpenter
> Signed-off-by: Fabio M. De Francesco
> ---
> drivers/staging/rtl8723bs/core/rtw_cmd.c | 4 +++-
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++-
> 3 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..7b6102a2bb2c 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -87,6 +87,8 @@ static struct _cmd_callback rtw_cmd_callback[] = {
> {GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
> };
>
> +u8 dummy_functioni(struct adapter *var0, u8 *var1) { return 0; }
> +
> static struct cmd_hdl wlancmds[] = {
> GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
> GEN_DRV_CMD_HANDLER(0, NULL)
> @@ -150,7 +152,7 @@ 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*/
> + GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), dummy_function)
> /*60*/
No, no. Don't create a dummy function. Do it like so:
GEN_DRV_CMD_HANDLER(0, NULL) /* 60 */
regards,
dan carpenter
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote: > Removed the led_blink_hdl() function (declaration and definition). > Declared dummy_function() in include/rtw_mlme_ext.h and defined it in > core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER > macro to make use of dummy_function(). No no no. If you want to remove is function declaration and use, then do it properly. The code is crazy, I agree, but it should not be difficult to just remove this correctly instead of papering over this mess. Also note that no one actually calls this function if you look at the logic here. It might take some good knowledge of C to unwind this crud, but once done, you should be able to "prove" it's not called and how to remove it correctly. And no, I'm not going to say how to do it, that's an exercise best left for the reader. But I will hint that this was done in the past, in 2014, in another driver in the tree with a codebase much like this one, so it shouldn't be hard to find an example of it. Only took me a few minutes... good luck! greg k-h
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> Removed the led_blink_hdl() function (declaration and definition).
> Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> macro to make use of dummy_function().
>
> Reported-by: Julia Lawall
> Suggested-by: Dan Carpenter
> Signed-off-by: Fabio M. De Francesco
> ---
> drivers/staging/rtl8723bs/core/rtw_cmd.c | 4 +++-
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
> drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++-
> 3 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..7b6102a2bb2c 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -87,6 +87,8 @@ static struct _cmd_callback rtw_cmd_callback[] = {
> {GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
> };
>
> +u8 dummy_functioni(struct adapter *var0, u8 *var1) { return 0; }
> +
I think that this won't compile
> static struct cmd_hdl wlancmds[] = {
> GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
> GEN_DRV_CMD_HANDLER(0, NULL)
> @@ -150,7 +152,7 @@ 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*/
> + GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), dummy_function)
> /*60*/
>
> 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 873d3792ac8e..963ea80083c8 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..57977da78eb3 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> @@ -745,11 +745,12 @@ 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);
>
> +/* Dummy function used to fill elements of an array of function pointers */
> +u8 dummy_function(struct adapter *, u8 *);
here 'dummy_function' prototype is declared
>
> #define GEN_DRV_CMD_HANDLER(size, cmd) {size, &cmd ## _hdl},
> #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> --
> 2.31.1
>
>
thank you,
fabio
[Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()
Removed the led_blink_hdl() function (declaration and definition).
Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
macro to make use of dummy_function().
Reported-by: Julia Lawall
Suggested-by: Dan Carpenter
Signed-off-by: Fabio M. De Francesco
---
drivers/staging/rtl8723bs/core/rtw_cmd.c | 4 +++-
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c| 9 -
drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++-
3 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 0297fbad7bce..7b6102a2bb2c 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -87,6 +87,8 @@ static struct _cmd_callback rtw_cmd_callback[] = {
{GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
};
+u8 dummy_functioni(struct adapter *var0, u8 *var1) { return 0; }
+
static struct cmd_hdl wlancmds[] = {
GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
GEN_DRV_CMD_HANDLER(0, NULL)
@@ -150,7 +152,7 @@ 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*/
+ GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), dummy_function)
/*60*/
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 873d3792ac8e..963ea80083c8 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..57977da78eb3 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
@@ -745,11 +745,12 @@ 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);
+/* Dummy function used to fill elements of an array of function pointers */
+u8 dummy_function(struct adapter *, u8 *);
#define GEN_DRV_CMD_HANDLER(size, cmd) {size, &cmd ## _hdl},
#define GEN_MLME_EXT_HANDLER(size, cmd){size, cmd},
--
2.31.1
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 09:59:36AM +0200, Fabio M. De Francesco wrote: > Fine. I'm about to submit a patch. > > Is there any formal means to acknowledge (in the patch) your contribution > to the resolution of this problem? It doesn't matter. Suggested-by. regards, dan carpenter
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Wednesday, April 14, 2021 9:00:25 AM CEST Dan Carpenter wrote:
> On Wed, Apr 14, 2021 at 08:33:48AM +0200, Fabio M. De Francesco wrote:
> > On Wednesday, April 14, 2021 7:21:50 AM CEST Dan Carpenter wrote:
> > > On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco
wrote:
> > > > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco
> >
> > wrote:
> > > > > > 1) The driver doesn't call that function from anywhere else
> > > > > > than
> > > > > > the
> > > > > > macro. 2) You have explained that the macro add its symbol to a
> > > > > > slot
> > > > > > in an array that would shift all the subsequent elements down
> > > > > > if
> > > > > > that
> > > > > > macro is not used exactly in the line where it is.
> > > > > > 3) Dan Carpenter said that that array is full of null functions
> > > > > > (or
> > > > > > empty slots?).
> > > > > >
> > > > > > Unless that function is called anonymously dereferencing its
> > > > > > address
> > > > > > from the position it occupies in the array, I'm not able to see
> > > > > > what
> > > > > > else means can any caller use.
> > > > > >
> > > > > > I know I have much less experience than you with C: what can go
> > > > > > wrong?
> > > > >
> > > > > Here's where the driver calls that function:
> > > > >
> > > > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl
> > > > >
> > > > > wlancmds[] = { drivers/staging/rtl8723bs/core/rtw_cmd.c:
> > > > > if
> > > > >
> > > > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:
> > > > > cmd_hdl
> > > > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > > >
> > > > OK, I had imagined an anonymous call from its location in the array
> > > > (as
> > > > I wrote in the last phrase of my message). However, I thought that
> > > > it
> > > > could have been an improbable possibility, not a real one.
> > > >
> > > > Linux uses a lot of interesting ideas that newcomers like me should
> > > > learn. Things here are trickier than they appear at first sight.
> > >
> > > One trick would be to build the Smatch cross function database.
> > >
> > > https://www.spinics.net/lists/smatch/msg00568.html
> > >
> > > Then you could do:
> > >
> > > $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> > > file | caller | function | type | parameter | key | value |
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c | rtw_cmd_thread |
> > > rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 |
> > > |
> > > uchar(*)(struct adapter*, uchar*)
> > > drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> > > rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 |
> > > |
> > > uchar(*)(struct adapter*, uchar*)
> > > drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> > > rtw_cmd_thread ptr cmd_hdl | BUF_SIZE | 1 |
> > > pbuf |
> > > 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
> > >
> > >
> > > Which says that led_blink_hdl() is called as a function pointer
> > > called
> > > "cmd_hdl" from rtw_cmd_thread().
> > >
> > > Hm... It says it can be called from either rtw_cmd_thread() function
> > > (the rtl8723bs or rtl8188eu version) which is not ideal. But also
> > > basically harmless so whatever...
> > >
> > > regards,
> > > dan carpenter
> >
> > Nice tool, thanks. I'll surely use it when it is needed to find out
> > which callers use a function pointer.
> >
> > However I cannot see how it can help in this context. That function
> > *does* something, even if I cannot understand why someone needs a
> > function to test the initialization of a pointer. Furthermore it is
> > actually called by rtw_cmd_thread() (as you found out by using smatch)
> > that expect one of the two possible values that led_blink_hdl()
> > returns.
> >
> > That said, what trick could I use for the purpose of getting rid of
> > that
> > function? At this point I'm not sure it could be made.
>
> If you look at how this is called:
>
> drivers/staging/rtl8723bs/core/rtw_cmd.c
>449 memcpy(pcmdbuf, pcmd->parmbuf, pcmd->cmdsz);
>450
>451 if (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
>452 cmd_hdl =
> wlancmds[pcmd->cmdcode].h2cfuns; 453
>454 if (cmd_hdl) {
>455 ret = cmd_hdl(pcmd->padapter,
> pcmdbuf); ^^^
>
>456 pcmd->res = ret;
>457 }
>458
>459 pcmdpriv->cmd_seq++;
>460 } else {
>461 pcmd->res = H2C_PARAMETERS_ERROR;
>462 }
>463
>464 cmd_hdl = NULL;
>
> The led_blink_hdl() function returns success if "pcmdbuf" is non-NULL
> and fail if it's NULL.
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 09:40:36AM +0200, Fabio Aiuto wrote:
> On Wed, Apr 14, 2021 at 08:21:50AM +0300, Dan Carpenter wrote:
> > On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> > > > > 1) The driver doesn't call that function from anywhere else than the
> > > > > macro. 2) You have explained that the macro add its symbol to a slot
> > > > > in an array that would shift all the subsequent elements down if that
> > > > > macro is not used exactly in the line where it is.
> > > > > 3) Dan Carpenter said that that array is full of null functions (or
> > > > > empty slots?).
> > > > >
> > > > > Unless that function is called anonymously dereferencing its address
> > > > > from the position it occupies in the array, I'm not able to see what
> > > > > else means can any caller use.
> > > > >
> > > > > I know I have much less experience than you with C: what can go wrong?
> > > >
> > > > Here's where the driver calls that function:
> > > >
> > > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl
> > > > wlancmds[]
> > > > = { drivers/staging/rtl8723bs/core/rtw_cmd.c: if
> > > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c: cmd_hdl
> > > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > > >
> > > OK, I had imagined an anonymous call from its location in the array (as I
> > > wrote in the last phrase of my message). However, I thought that it could
> > > have been an improbable possibility, not a real one.
> > >
> > > Linux uses a lot of interesting ideas that newcomers like me should
> > > learn.
> > > Things here are trickier than they appear at first sight.
> >
> > One trick would be to build the Smatch cross function database.
> >
> > https://www.spinics.net/lists/smatch/msg00568.html
> >
> > Then you could do:
> >
> > $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> > file | caller | function | type | parameter | key | value |
> > drivers/staging/rtl8723bs/core/rtw_cmd.c | rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
> > uchar(*)(struct adapter*, uchar*)
> > drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
> > uchar(*)(struct adapter*, uchar*)
> > drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl | BUF_SIZE | 1 |pbuf |
> > 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
> >
> >
> > Which says that led_blink_hdl() is called as a function pointer called
> > "cmd_hdl" from rtw_cmd_thread().
> >
> > Hm... It says it can be called from either rtw_cmd_thread() function
> > (the rtl8723bs or rtl8188eu version) which is not ideal. But also
> > basically harmless so whatever...
> >
> > regards,
> > dan carpenter
> >
>
> very powerful tool.
>
> I tried this:
>
> fabio@agape:~/src/git/kernels/staging$
> ~/src/git/smatch/smatch_data/db/smdb.py led_blink_hdl
> Traceback (most recent call last):
> File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 725, in
>
> print_caller_info("", func)
> File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 366, in
> print_caller_info
> ptrs = get_function_pointers(func)
> File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 53, in
> get_function_pointers
> get_function_pointers_helper(func)
> File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 38, in
> get_function_pointers_helper
> cur.execute("select distinct ptr from function_ptr where function =
> '%s';" %(func))
> sqlite3.OperationalError: no such table: function_ptr
>
> I run smatch version 1.71 on Debian Buster machine
>
It takes a few hours to build the DB. The instructions are in the
email.
~/path/to/smatch/smatch_scripts/build_kernel_data.sh
regards,
dan carpenter
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 08:21:50AM +0300, Dan Carpenter wrote:
> On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> > > > 1) The driver doesn't call that function from anywhere else than the
> > > > macro. 2) You have explained that the macro add its symbol to a slot
> > > > in an array that would shift all the subsequent elements down if that
> > > > macro is not used exactly in the line where it is.
> > > > 3) Dan Carpenter said that that array is full of null functions (or
> > > > empty slots?).
> > > >
> > > > Unless that function is called anonymously dereferencing its address
> > > > from the position it occupies in the array, I'm not able to see what
> > > > else means can any caller use.
> > > >
> > > > I know I have much less experience than you with C: what can go wrong?
> > >
> > > Here's where the driver calls that function:
> > >
> > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[]
> > > = { drivers/staging/rtl8723bs/core/rtw_cmd.c: if
> > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c: cmd_hdl
> > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > >
> > OK, I had imagined an anonymous call from its location in the array (as I
> > wrote in the last phrase of my message). However, I thought that it could
> > have been an improbable possibility, not a real one.
> >
> > Linux uses a lot of interesting ideas that newcomers like me should learn.
> > Things here are trickier than they appear at first sight.
>
> One trick would be to build the Smatch cross function database.
>
> https://www.spinics.net/lists/smatch/msg00568.html
>
> Then you could do:
>
> $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> file | caller | function | type | parameter | key | value |
> drivers/staging/rtl8723bs/core/rtw_cmd.c | rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
> uchar(*)(struct adapter*, uchar*)
> drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
> uchar(*)(struct adapter*, uchar*)
> drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl | BUF_SIZE | 1 |pbuf |
> 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
>
>
> Which says that led_blink_hdl() is called as a function pointer called
> "cmd_hdl" from rtw_cmd_thread().
>
> Hm... It says it can be called from either rtw_cmd_thread() function
> (the rtl8723bs or rtl8188eu version) which is not ideal. But also
> basically harmless so whatever...
>
> regards,
> dan carpenter
>
very powerful tool.
I tried this:
fabio@agape:~/src/git/kernels/staging$ ~/src/git/smatch/smatch_data/db/smdb.py
led_blink_hdl
Traceback (most recent call last):
File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 725, in
print_caller_info("", func)
File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 366, in
print_caller_info
ptrs = get_function_pointers(func)
File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 53, in
get_function_pointers
get_function_pointers_helper(func)
File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 38, in
get_function_pointers_helper
cur.execute("select distinct ptr from function_ptr where function = '%s';"
%(func))
sqlite3.OperationalError: no such table: function_ptr
I run smatch version 1.71 on Debian Buster machine
what's happened?
thanks in advance,
fabio
Re: [Outreachy kernel] Re: [PATCH v3 4/4] staging: media: intel-ipu3: remove space before tabs
On Wed, Apr 14, 2021 at 12:05:04AM +0200, Julia Lawall wrote:
>
>
> 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.
v2 of this patch can be used as well, it's fine. (I missed this change in
v3.)
--
Sakari Ailus
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Wed, Apr 14, 2021 at 08:33:48AM +0200, Fabio M. De Francesco wrote:
> On Wednesday, April 14, 2021 7:21:50 AM CEST Dan Carpenter wrote:
> > On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco
> wrote:
> > > > > 1) The driver doesn't call that function from anywhere else than
> > > > > the
> > > > > macro. 2) You have explained that the macro add its symbol to a
> > > > > slot
> > > > > in an array that would shift all the subsequent elements down if
> > > > > that
> > > > > macro is not used exactly in the line where it is.
> > > > > 3) Dan Carpenter said that that array is full of null functions (or
> > > > > empty slots?).
> > > > >
> > > > > Unless that function is called anonymously dereferencing its
> > > > > address
> > > > > from the position it occupies in the array, I'm not able to see
> > > > > what
> > > > > else means can any caller use.
> > > > >
> > > > > I know I have much less experience than you with C: what can go
> > > > > wrong?
> > > >
> > > > Here's where the driver calls that function:
> > > >
> > > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl
> > > > wlancmds[] = { drivers/staging/rtl8723bs/core/rtw_cmd.c:
> > > > if
> > > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:
> > > > cmd_hdl
> > > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > >
> > > OK, I had imagined an anonymous call from its location in the array (as
> > > I wrote in the last phrase of my message). However, I thought that it
> > > could have been an improbable possibility, not a real one.
> > >
> > > Linux uses a lot of interesting ideas that newcomers like me should
> > > learn. Things here are trickier than they appear at first sight.
> >
> > One trick would be to build the Smatch cross function database.
> >
> > https://www.spinics.net/lists/smatch/msg00568.html
> >
> > Then you could do:
> >
> > $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> > file | caller | function | type | parameter | key | value |
> > drivers/staging/rtl8723bs/core/rtw_cmd.c | rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
> > uchar(*)(struct adapter*, uchar*)
> > drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
> > uchar(*)(struct adapter*, uchar*)
> > drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl | BUF_SIZE | 1 |pbuf |
> > 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
> >
> >
> > Which says that led_blink_hdl() is called as a function pointer called
> > "cmd_hdl" from rtw_cmd_thread().
> >
> > Hm... It says it can be called from either rtw_cmd_thread() function
> > (the rtl8723bs or rtl8188eu version) which is not ideal. But also
> > basically harmless so whatever...
> >
> > regards,
> > dan carpenter
> >
> Nice tool, thanks. I'll surely use it when it is needed to find out which
> callers use a function pointer.
>
> However I cannot see how it can help in this context. That function *does*
> something, even if I cannot understand why someone needs a function to test
> the initialization of a pointer. Furthermore it is actually called by
> rtw_cmd_thread() (as you found out by using smatch) that expect one of the
> two possible values that led_blink_hdl() returns.
>
> That said, what trick could I use for the purpose of getting rid of that
> function? At this point I'm not sure it could be made.
If you look at how this is called:
drivers/staging/rtl8723bs/core/rtw_cmd.c
449 memcpy(pcmdbuf, pcmd->parmbuf, pcmd->cmdsz);
450
451 if (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
452 cmd_hdl = wlancmds[pcmd->cmdcode].h2cfuns;
453
454 if (cmd_hdl) {
455 ret = cmd_hdl(pcmd->padapter, pcmdbuf);
^^^
456 pcmd->res = ret;
457 }
458
459 pcmdpriv->cmd_seq++;
460 } else {
461 pcmd->res = H2C_PARAMETERS_ERROR;
462 }
463
464 cmd_hdl = NULL;
The led_blink_hdl() function returns success if "pcmdbuf" is non-NULL
and fail if it's NULL. "pcmdbuf" is never supposed to be NULL. (The
"supposed" caveat is because there may be a race in rtw_sdio_if1_init()
which briefly allows a NULL "pcmdbuf", but that is an unrelated bug).
Anyway, there is no point to the led_blink_hdl() function. Likely
they intended
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Wednesday, April 14, 2021 7:21:50 AM CEST Dan Carpenter wrote:
> On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco
wrote:
> > > > 1) The driver doesn't call that function from anywhere else than
> > > > the
> > > > macro. 2) You have explained that the macro add its symbol to a
> > > > slot
> > > > in an array that would shift all the subsequent elements down if
> > > > that
> > > > macro is not used exactly in the line where it is.
> > > > 3) Dan Carpenter said that that array is full of null functions (or
> > > > empty slots?).
> > > >
> > > > Unless that function is called anonymously dereferencing its
> > > > address
> > > > from the position it occupies in the array, I'm not able to see
> > > > what
> > > > else means can any caller use.
> > > >
> > > > I know I have much less experience than you with C: what can go
> > > > wrong?
> > >
> > > Here's where the driver calls that function:
> > >
> > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl
> > > wlancmds[] = { drivers/staging/rtl8723bs/core/rtw_cmd.c:
> > > if
> > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c:
> > > cmd_hdl
> > > = wlancmds[pcmd->cmdcode].h2cfuns;
> >
> > OK, I had imagined an anonymous call from its location in the array (as
> > I wrote in the last phrase of my message). However, I thought that it
> > could have been an improbable possibility, not a real one.
> >
> > Linux uses a lot of interesting ideas that newcomers like me should
> > learn. Things here are trickier than they appear at first sight.
>
> One trick would be to build the Smatch cross function database.
>
> https://www.spinics.net/lists/smatch/msg00568.html
>
> Then you could do:
>
> $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> file | caller | function | type | parameter | key | value |
> drivers/staging/rtl8723bs/core/rtw_cmd.c | rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
> uchar(*)(struct adapter*, uchar*)
> drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
> uchar(*)(struct adapter*, uchar*)
> drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl | BUF_SIZE | 1 |pbuf |
> 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
>
>
> Which says that led_blink_hdl() is called as a function pointer called
> "cmd_hdl" from rtw_cmd_thread().
>
> Hm... It says it can be called from either rtw_cmd_thread() function
> (the rtl8723bs or rtl8188eu version) which is not ideal. But also
> basically harmless so whatever...
>
> regards,
> dan carpenter
>
Nice tool, thanks. I'll surely use it when it is needed to find out which
callers use a function pointer.
However I cannot see how it can help in this context. That function *does*
something, even if I cannot understand why someone needs a function to test
the initialization of a pointer. Furthermore it is actually called by
rtw_cmd_thread() (as you found out by using smatch) that expect one of the
two possible values that led_blink_hdl() returns.
That said, what trick could I use for the purpose of getting rid of that
function? At this point I'm not sure it could be made.
Regards,
Fabio
[Outreachy kernel] [PATCH v2] staging: rtl8723bs: hal: Remove four set but not used variables
Removed four variables that were set but not used.
Signed-off-by: Fabio M. De Francesco
---
Changes from v1: deleted one blank line.
drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 77f8353c5ce5..63f7f673aefb 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -3199,13 +3199,10 @@ static void hw_var_set_mlme_join(struct adapter
*padapter, u8 variable, u8 *val)
void CCX_FwC2HTxRpt_8723b(struct adapter *padapter, u8 *pdata, u8 len)
{
- u8 seq_no;
#defineGET_8723B_C2H_TX_RPT_LIFE_TIME_OVER(_Header)
LE_BITS_TO_1BYTE((_Header + 0), 6, 1)
#defineGET_8723B_C2H_TX_RPT_RETRY_OVER(_Header)
LE_BITS_TO_1BYTE((_Header + 0), 7, 1)
- seq_no = *(pdata+6);
-
if (GET_8723B_C2H_TX_RPT_RETRY_OVER(pdata) |
GET_8723B_C2H_TX_RPT_LIFE_TIME_OVER(pdata)) {
rtw_ack_tx_done(&padapter->xmitpriv,
RTW_SCTX_DONE_CCX_PKT_FAIL);
}
@@ -3357,17 +3354,15 @@ void SetHwReg8723B(struct adapter *padapter, u8
variable, u8 *val)
case HW_VAR_BASIC_RATE:
{
struct mlme_ext_info *mlmext_info =
&padapter->mlmeextpriv.mlmext_info;
- u16 input_b = 0, masked = 0, ioted = 0, BrateCfg = 0;
+ u16 BrateCfg = 0;
u16 rrsr_2g_force_mask = (RRSR_11M|RRSR_5_5M|RRSR_1M);
u16 rrsr_2g_allow_mask =
(RRSR_24M|RRSR_12M|RRSR_6M|RRSR_CCK_RATES);
HalSetBrateCfg(padapter, val, &BrateCfg);
- input_b = BrateCfg;
/* apply force and allow mask */
BrateCfg |= rrsr_2g_force_mask;
BrateCfg &= rrsr_2g_allow_mask;
- masked = BrateCfg;
/* IOT consideration */
if (mlmext_info->assoc_AP_vendor == HT_IOT_PEER_CISCO) {
@@ -3375,7 +3370,6 @@ void SetHwReg8723B(struct adapter *padapter, u8 variable,
u8 *val)
if ((BrateCfg & (RRSR_24M|RRSR_12M|RRSR_6M)) == 0)
BrateCfg |= RRSR_6M;
}
- ioted = BrateCfg;
pHalData->BasicRateSet = BrateCfg;
--
2.31.1
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> > > 1) The driver doesn't call that function from anywhere else than the
> > > macro. 2) You have explained that the macro add its symbol to a slot
> > > in an array that would shift all the subsequent elements down if that
> > > macro is not used exactly in the line where it is.
> > > 3) Dan Carpenter said that that array is full of null functions (or
> > > empty slots?).
> > >
> > > Unless that function is called anonymously dereferencing its address
> > > from the position it occupies in the array, I'm not able to see what
> > > else means can any caller use.
> > >
> > > I know I have much less experience than you with C: what can go wrong?
> >
> > Here's where the driver calls that function:
> >
> > $ git grep wlancmds drivers/staging/rtl8723bs/
> > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[]
> > = { drivers/staging/rtl8723bs/core/rtw_cmd.c: if
> > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > drivers/staging/rtl8723bs/core/rtw_cmd.c: cmd_hdl
> > = wlancmds[pcmd->cmdcode].h2cfuns;
> >
> OK, I had imagined an anonymous call from its location in the array (as I
> wrote in the last phrase of my message). However, I thought that it could
> have been an improbable possibility, not a real one.
>
> Linux uses a lot of interesting ideas that newcomers like me should learn.
> Things here are trickier than they appear at first sight.
One trick would be to build the Smatch cross function database.
https://www.spinics.net/lists/smatch/msg00568.html
Then you could do:
$ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
file | caller | function | type | parameter | key | value |
drivers/staging/rtl8723bs/core/rtw_cmd.c | rtw_cmd_thread |
rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
uchar(*)(struct adapter*, uchar*)
drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
rtw_cmd_thread ptr cmd_hdl | INTERNAL | -1 | |
uchar(*)(struct adapter*, uchar*)
drivers/staging/rtl8188eu/core/rtw_cmd.c | rtw_cmd_thread |
rtw_cmd_thread ptr cmd_hdl | BUF_SIZE | 1 |pbuf |
1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
Which says that led_blink_hdl() is called as a function pointer called
"cmd_hdl" from rtw_cmd_thread().
Hm... It says it can be called from either rtw_cmd_thread() function
(the rtl8723bs or rtl8188eu version) which is not ideal. But also
basically harmless so whatever...
regards,
dan carpenter
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: hal: Remove four set but not used variables
On Tue, Apr 13, 2021 at 08:42:22PM +0200, Fabio M. De Francesco wrote:
> Removed four variables that were set but not used.
>
> Signed-off-by: Fabio M. De Francesco
> ---
> drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> index 77f8353c5ce5..fad6a3bfe07c 100644
> --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> @@ -3199,12 +3199,10 @@ static void hw_var_set_mlme_join(struct adapter
> *padapter, u8 variable, u8 *val)
>
> void CCX_FwC2HTxRpt_8723b(struct adapter *padapter, u8 *pdata, u8 len)
> {
> - u8 seq_no;
>
> #define GET_8723B_C2H_TX_RPT_LIFE_TIME_OVER(_Header)
> LE_BITS_TO_1BYTE((_Header + 0), 6, 1)
> #define GET_8723B_C2H_TX_RPT_RETRY_OVER(_Header)
> LE_BITS_TO_1BYTE((_Header + 0), 7, 1)
>
> - seq_no = *(pdata+6);
>
> if (GET_8723B_C2H_TX_RPT_RETRY_OVER(pdata) |
> GET_8723B_C2H_TX_RPT_LIFE_TIME_OVER(pdata)) {
Now we have two blank lines in a row. Please delete one.
regards,
dan carpenter
Re: [Outreachy kernel] Re: [PATCH v3 4/4] staging: media: intel-ipu3: remove space before tabs
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()
On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> > 1) The driver doesn't call that function from anywhere else than the
> > macro. 2) You have explained that the macro add its symbol to a slot
> > in an array that would shift all the subsequent elements down if that
> > macro is not used exactly in the line where it is.
> > 3) Dan Carpenter said that that array is full of null functions (or
> > empty slots?).
> >
> > Unless that function is called anonymously dereferencing its address
> > from the position it occupies in the array, I'm not able to see what
> > else means can any caller use.
> >
> > I know I have much less experience than you with C: what can go wrong?
>
> Here's where the driver calls that function:
>
> $ git grep wlancmds drivers/staging/rtl8723bs/
> drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[]
> = { drivers/staging/rtl8723bs/core/rtw_cmd.c: if
> (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> drivers/staging/rtl8723bs/core/rtw_cmd.c: cmd_hdl
> = wlancmds[pcmd->cmdcode].h2cfuns;
>
OK, I had imagined an anonymous call from its location in the array (as I
wrote in the last phrase of my message). However, I thought that it could
have been an improbable possibility, not a real one.
Linux uses a lot of interesting ideas that newcomers like me should learn.
Things here are trickier than they appear at first sight.
Thanks,
Fabio
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> 1) The driver doesn't call that function from anywhere else than the macro.
> 2) You have explained that the macro add its symbol to a slot in an array
> that would shift all the subsequent elements down if that macro is not used
> exactly in the line where it is.
> 3) Dan Carpenter said that that array is full of null functions (or empty
> slots?).
>
> Unless that function is called anonymously dereferencing its address from
> the position it occupies in the array, I'm not able to see what else means
> can any caller use.
>
> I know I have much less experience than you with C: what can go wrong?
Here's where the driver calls that function:
$ git grep wlancmds drivers/staging/rtl8723bs/
drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[] = {
drivers/staging/rtl8723bs/core/rtw_cmd.c: if (pcmd->cmdcode <
ARRAY_SIZE(wlancmds)) {
drivers/staging/rtl8723bs/core/rtw_cmd.c: cmd_hdl =
wlancmds[pcmd->cmdcode].h2cfuns;
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
On Tuesday, April 13, 2021 9:25:05 PM CEST Julia Lawall wrote:
> 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'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
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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'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?
> >
> No, I have not, but perhaps is not relevant.
>
> I want to give to the macro the name of an empty function that I define in
> the same header where there the prototype of led_blink_hdl() is defined
> now: something like "u8 empty_function { return 0; }"
>
> Can it work
> What do you think about it?
The previous function didn't return 0. It returned something else.
To do anything this, you have to fin
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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'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?
>
No, I have not, but perhaps is not relevant.
I want to give to the macro the name of an empty function that I define in
the same header where there the prototype of led_blink_hdl() is defined
now: something like "u8 empty_function { return 0; }"
Can it work
What do you think about it?
Fabio
>
> julia
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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
[Outreachy kernel] [PATCH] staging: rtl8723bs: hal: Remove four set but not used variables
Removed four variables that were set but not used.
Signed-off-by: Fabio M. De Francesco
---
drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 77f8353c5ce5..fad6a3bfe07c 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -3199,12 +3199,10 @@ static void hw_var_set_mlme_join(struct adapter
*padapter, u8 variable, u8 *val)
void CCX_FwC2HTxRpt_8723b(struct adapter *padapter, u8 *pdata, u8 len)
{
- u8 seq_no;
#defineGET_8723B_C2H_TX_RPT_LIFE_TIME_OVER(_Header)
LE_BITS_TO_1BYTE((_Header + 0), 6, 1)
#defineGET_8723B_C2H_TX_RPT_RETRY_OVER(_Header)
LE_BITS_TO_1BYTE((_Header + 0), 7, 1)
- seq_no = *(pdata+6);
if (GET_8723B_C2H_TX_RPT_RETRY_OVER(pdata) |
GET_8723B_C2H_TX_RPT_LIFE_TIME_OVER(pdata)) {
rtw_ack_tx_done(&padapter->xmitpriv,
RTW_SCTX_DONE_CCX_PKT_FAIL);
@@ -3357,17 +3355,15 @@ void SetHwReg8723B(struct adapter *padapter, u8
variable, u8 *val)
case HW_VAR_BASIC_RATE:
{
struct mlme_ext_info *mlmext_info =
&padapter->mlmeextpriv.mlmext_info;
- u16 input_b = 0, masked = 0, ioted = 0, BrateCfg = 0;
+ u16 BrateCfg = 0;
u16 rrsr_2g_force_mask = (RRSR_11M|RRSR_5_5M|RRSR_1M);
u16 rrsr_2g_allow_mask =
(RRSR_24M|RRSR_12M|RRSR_6M|RRSR_CCK_RATES);
HalSetBrateCfg(padapter, val, &BrateCfg);
- input_b = BrateCfg;
/* apply force and allow mask */
BrateCfg |= rrsr_2g_force_mask;
BrateCfg &= rrsr_2g_allow_mask;
- masked = BrateCfg;
/* IOT consideration */
if (mlmext_info->assoc_AP_vendor == HT_IOT_PEER_CISCO) {
@@ -3375,7 +3371,6 @@ void SetHwReg8723B(struct adapter *padapter, u8 variable,
u8 *val)
if ((BrateCfg & (RRSR_24M|RRSR_12M|RRSR_6M)) == 0)
BrateCfg |= RRSR_6M;
}
- ioted = BrateCfg;
pHalData->BasicRateSet = BrateCfg;
--
2.31.1
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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.
Thanks a lot,
Fabio
>
> regards,
> dan carpenter
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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.
regards,
dan carpenter
Re: [Outreachy kernel][PATCH 1/2] staging: media: omap4iss: Align line break to the open parenthesis in file iss.c
Em ter, 2021-04-13 às 16:32 +0200, Hans Verkuil escreveu:
> On 09/04/2021 21:01, 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 <
> > alinesantanacorde...@gmail.com>
>
> Obsolete, a similar patch from Beatriz Martins de Carvalho <
> martinsdecarvalhobeat...@gmail.com>
> has already been applied in the media subsystem tree.
>
> Regards,
>
> Hans
>
Okay, thank you for letting me know.
Aline
> > ---
> > drivers/staging/media/omap4iss/iss.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/omap4iss/iss.c
> > b/drivers/staging/media/omap4iss/iss.c
> > index dae9073..c89f268a 100644
> > --- a/drivers/staging/media/omap4iss/iss.c
> > +++ b/drivers/staging/media/omap4iss/iss.c
> > @@ -960,7 +960,7 @@ iss_register_subdev_group(struct iss_device
> > *iss,
> > }
> >
> > subdev = v4l2_i2c_new_subdev_board(&iss->v4l2_dev,
> > adapter,
> > - board_info->board_info, NULL);
> > + board_info-
> > >board_info, NULL);
> > if (!subdev) {
> > dev_err(iss->dev, "Unable to register
> > subdev %s\n",
> > board_info->board_info->type);
> >
>
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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.
OK, h2c_msg_hdl() cannot be deleted.
Thanks,
Fabio
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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()
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?
Thanks for your kind help,
Fabio
>
> 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: Remove useless led_blink_hdl()
On Tue, Apr 13, 2021 at 05:59:08PM +0200, 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. But you do not actually do that here. Why not? You just removed something, does it still work properly with that removal? > > Signed-off-by: Fabio M. De Francesco Why the leading ":" in your subject line? > --- > 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(-) Does this patch require some other patch to be applied first? thanks, greg k-h
Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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.
>
[Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()
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*/
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
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: core: Remove unused but set variable
On Tuesday, April 13, 2021 5:16:17 PM CEST Julia Lawall wrote:
> 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
>
Actually, it is completely useless.
We should ask the original authors for explanations :)
I'm about to grep the whole driver for the purpose to check if there are
callers elsewhere and then delete any call and the function itself.
Thanks,
Fabio
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: core: Remove unused but set variable
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: [Outreachy kernel] [PATCH] staging: rtl8192u: ieee80211: Replaced strncpy() with strscpy()
On Tue, Apr 13, 2021 at 03:24:31PM +0200, Fabio M. De Francesco wrote: > On Tuesday, April 13, 2021 3:16:17 PM CEST Greg Kroah-Hartman wrote: > > On Tue, Apr 13, 2021 at 03:12:02PM +0200, Fabio M. De Francesco wrote: > > > On Tuesday, April 13, 2021 2:59:29 PM CEST Greg Kroah-Hartman wrote: > > > > On Tue, Apr 13, 2021 at 02:30:41PM +0200, Fabio M. De Francesco > wrote: > > > > > Replaced strncpy() with strscpy() because of compilation time > > > > > warnings > > > > > about possible truncation of output [-Wstringop-truncation]. > > > > > > > > build warnings? What build warnings? > > > > > > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:1388:5: warning: > > > ‘strncpy’ output may be truncated copying 32 bytes from a string of > > > length 32 [-Wstringop-truncation] > > > > > > 1388 | strncpy(tmp_ssid, ieee->current_network.ssid, > > > > > > IW_ESSID_MAX_SIZE); > > > > That's implying that there is a real bug here, not that just replacing > > it with a different call is going to solve this, right? > > > > And how do you see that, I can't see that in my builds. > > > I see that with flag W=1 like in > make -j8 drivers/staging/rtl8192u/ W=1 Ah, no one "normal" builds with "W=1" :) > However I also think it's not a real issue in this context. > Just that strscpy() is preferred and get rid of warnings. > You only can judge if a patch is worth. > I just thought that gcc is (mostly) right in pointing out warnings like > that. Check and verify if this is wrong as-is and if so, then replace it properly. Don't just blindly to a search/replace, as that's not ok here. thanks, greg k-h
[Outreachy kernel] [PATCH] staging: rtl8723bs: core: Remove unused but set variable
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;
}
--
2.31.1
Re: [Outreachy kernel][PATCH 1/2] staging: media: omap4iss: Align line break to the open parenthesis in file iss.c
On 09/04/2021 21:01, 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
Obsolete, a similar patch from Beatriz Martins de Carvalho
has already been applied in the media subsystem tree.
Regards,
Hans
> ---
> drivers/staging/media/omap4iss/iss.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss.c
> b/drivers/staging/media/omap4iss/iss.c
> index dae9073..c89f268a 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -960,7 +960,7 @@ iss_register_subdev_group(struct iss_device *iss,
> }
>
> subdev = v4l2_i2c_new_subdev_board(&iss->v4l2_dev, adapter,
> - board_info->board_info, NULL);
> +board_info->board_info,
> NULL);
> if (!subdev) {
> dev_err(iss->dev, "Unable to register subdev %s\n",
> board_info->board_info->type);
>
Re: [Outreachy kernel] [PATCH] staging: rtl8192u: ieee80211: Replaced strncpy() with strscpy()
On Tuesday, April 13, 2021 3:16:17 PM CEST Greg Kroah-Hartman wrote:
> On Tue, Apr 13, 2021 at 03:12:02PM +0200, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 2:59:29 PM CEST Greg Kroah-Hartman wrote:
> > > On Tue, Apr 13, 2021 at 02:30:41PM +0200, Fabio M. De Francesco
wrote:
> > > > Replaced strncpy() with strscpy() because of compilation time
> > > > warnings
> > > > about possible truncation of output [-Wstringop-truncation].
> > >
> > > build warnings? What build warnings?
> >
> > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:1388:5: warning:
> > ‘strncpy’ output may be truncated copying 32 bytes from a string of
> > length 32 [-Wstringop-truncation]
> >
> > 1388 | strncpy(tmp_ssid, ieee->current_network.ssid,
> >
> > IW_ESSID_MAX_SIZE);
>
> That's implying that there is a real bug here, not that just replacing
> it with a different call is going to solve this, right?
>
> And how do you see that, I can't see that in my builds.
>
I see that with flag W=1 like in
make -j8 drivers/staging/rtl8192u/ W=1
However I also think it's not a real issue in this context.
Just that strscpy() is preferred and get rid of warnings.
You only can judge if a patch is worth.
I just thought that gcc is (mostly) right in pointing out warnings like
that.
Thanks,
Fabio
>
> > > > Furthermore, according to the Linux official documentation,
> > > > strscpy()
> > > > is
> > > > preferred to strncpy.
> > > >
> > > > Signed-off-by: Fabio M. De Francesco
> > > > ---
> > > >
> > > > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > > > b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c index
> > > > 25ea8e1b6b65..aa58eedf5e86 100644
> > > > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > > > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > > > @@ -1385,12 +1385,12 @@ inline void
> > > > ieee80211_softmac_new_net(struct
> > > > ieee80211_device *ieee, struct ieee>
> > > >
> > > > * essid provided by the user.
> > > > */
> > > >
> > > > if (!ssidbroad) {
> > > >
> > > > - strncpy(tmp_ssid, ieee-
> > >
> > >current_network.ssid, IW_ESSID_MAX_SIZE);
> > >
> > > > + strscpy(tmp_ssid, ieee-
> > >
> > >current_network.ssid, IW_ESSID_MAX_SIZE);
> > >
> > > Are you sure you can just replace this like this?
> >
> > I surely was... but now I'm not anymore, since your review :)
> >
> > Maybe you mean I have to check possible return of -E2BIG?
> > Did you mean something else?
> > May you please elaborate further?
>
> If it was as simple as search/replace, we would have already done that
> on the whole codebase at once. It's not that simple :)
>
> thanks,
>
> greg k-h
Re: [Outreachy kernel] [PATCH] staging: rtl8192u: ieee80211: Replaced strncpy() with strscpy()
On Tue, Apr 13, 2021 at 03:12:02PM +0200, Fabio M. De Francesco wrote:
> On Tuesday, April 13, 2021 2:59:29 PM CEST Greg Kroah-Hartman wrote:
> > On Tue, Apr 13, 2021 at 02:30:41PM +0200, Fabio M. De Francesco wrote:
> > > Replaced strncpy() with strscpy() because of compilation time warnings
> > > about possible truncation of output [-Wstringop-truncation].
> >
> > build warnings? What build warnings?
> >
> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:1388:5: warning:
> ‘strncpy’ output may be truncated copying 32 bytes from a string of length
> 32 [-Wstringop-truncation]
> 1388 | strncpy(tmp_ssid, ieee->current_network.ssid,
> IW_ESSID_MAX_SIZE);
That's implying that there is a real bug here, not that just replacing
it with a different call is going to solve this, right?
And how do you see that, I can't see that in my builds.
> >
> > > Furthermore, according to the Linux official documentation, strscpy()
> > > is
> > > preferred to strncpy.
> > >
> > > Signed-off-by: Fabio M. De Francesco
> > > ---
> > >
> > > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > > b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c index
> > > 25ea8e1b6b65..aa58eedf5e86 100644
> > > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > > @@ -1385,12 +1385,12 @@ inline void ieee80211_softmac_new_net(struct
> > > ieee80211_device *ieee, struct ieee>
> > >* essid provided by the user.
> > >*/
> > >
> > > if (!ssidbroad) {
> > >
> > > - strncpy(tmp_ssid, ieee-
> >current_network.ssid, IW_ESSID_MAX_SIZE);
> > > + strscpy(tmp_ssid, ieee-
> >current_network.ssid, IW_ESSID_MAX_SIZE);
> >
> > Are you sure you can just replace this like this?
> >
> I surely was... but now I'm not anymore, since your review :)
>
> Maybe you mean I have to check possible return of -E2BIG?
> Did you mean something else?
> May you please elaborate further?
If it was as simple as search/replace, we would have already done that
on the whole codebase at once. It's not that simple :)
thanks,
greg k-h
Re: [Outreachy kernel] [PATCH] staging: rtl8192u: ieee80211: Replaced strncpy() with strscpy()
On Tuesday, April 13, 2021 2:59:29 PM CEST Greg Kroah-Hartman wrote:
> On Tue, Apr 13, 2021 at 02:30:41PM +0200, Fabio M. De Francesco wrote:
> > Replaced strncpy() with strscpy() because of compilation time warnings
> > about possible truncation of output [-Wstringop-truncation].
>
> build warnings? What build warnings?
>
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:1388:5: warning:
‘strncpy’ output may be truncated copying 32 bytes from a string of length
32 [-Wstringop-truncation]
1388 | strncpy(tmp_ssid, ieee->current_network.ssid,
IW_ESSID_MAX_SIZE);
>
> > Furthermore, according to the Linux official documentation, strscpy()
> > is
> > preferred to strncpy.
> >
> > Signed-off-by: Fabio M. De Francesco
> > ---
> >
> > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c index
> > 25ea8e1b6b65..aa58eedf5e86 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > @@ -1385,12 +1385,12 @@ inline void ieee80211_softmac_new_net(struct
> > ieee80211_device *ieee, struct ieee>
> > * essid provided by the user.
> > */
> >
> > if (!ssidbroad) {
> >
> > - strncpy(tmp_ssid, ieee-
>current_network.ssid, IW_ESSID_MAX_SIZE);
> > + strscpy(tmp_ssid, ieee-
>current_network.ssid, IW_ESSID_MAX_SIZE);
>
> Are you sure you can just replace this like this?
>
I surely was... but now I'm not anymore, since your review :)
Maybe you mean I have to check possible return of -E2BIG?
Did you mean something else?
May you please elaborate further?
Thanks,
Fabio
>
> > tmp_ssid_len = ieee-
>current_network.ssid_len;
> >
> > }
> > memcpy(&ieee->current_network, net,
sizeof(struct
> > ieee80211_network));
> >
> > - strncpy(ieee->current_network.ssid,
tmp_ssid, IW_ESSID_MAX_SIZE);
> > + strscpy(ieee->current_network.ssid,
tmp_ssid, IW_ESSID_MAX_SIZE);
>
> Same here, are you sure?
>
> thanks,
>
> greg k-h
Re: [Outreachy kernel] [PATCH] staging: rtl8192u: ieee80211: Replaced strncpy() with strscpy()
On Tue, Apr 13, 2021 at 02:30:41PM +0200, Fabio M. De Francesco wrote:
> Replaced strncpy() with strscpy() because of compilation time warnings
> about possible truncation of output [-Wstringop-truncation].
build warnings? What build warnings?
> Furthermore, according to the Linux official documentation, strscpy() is
> preferred to strncpy.
>
> Signed-off-by: Fabio M. De Francesco
> ---
> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> index 25ea8e1b6b65..aa58eedf5e86 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> @@ -1385,12 +1385,12 @@ inline void ieee80211_softmac_new_net(struct
> ieee80211_device *ieee, struct ieee
>* essid provided by the user.
>*/
> if (!ssidbroad) {
> - strncpy(tmp_ssid, ieee->current_network.ssid,
> IW_ESSID_MAX_SIZE);
> + strscpy(tmp_ssid, ieee->current_network.ssid,
> IW_ESSID_MAX_SIZE);
Are you sure you can just replace this like this?
> tmp_ssid_len = ieee->current_network.ssid_len;
> }
> memcpy(&ieee->current_network, net, sizeof(struct
> ieee80211_network));
>
> - strncpy(ieee->current_network.ssid, tmp_ssid,
> IW_ESSID_MAX_SIZE);
> + strscpy(ieee->current_network.ssid, tmp_ssid,
> IW_ESSID_MAX_SIZE);
Same here, are you sure?
thanks,
greg k-h
Re: [Outreachy kernel] Re: [PATCH v2 3/4] staging: media: intel-ipu3: reduce length of line
On Tuesday, April 13, 2021 12:56:36 PM CEST Mitali Borkar wrote:
> On Tue, Apr 13, 2021 at 01:44:32PM +0300, Sakari Ailus wrote:
> > On Tue, Apr 13, 2021 at 04:13:04PM +0530, Mitali Borkar wrote:
> > > On Tue, Apr 13, 2021 at 01:01:34PM +0300, Sakari Ailus wrote:
> > > > Hi Mitali,
> > > >
> > > > Thanks for the update.
> > > >
> > > > On Tue, Apr 13, 2021 at 10:46:06AM +0530, Mitali Borkar wrote:
> > > > > Reduced length of the line under 80 characters to meet
> > > > > linux-kernel
> > > > > coding style.
> > > > >
> > > > > Signed-off-by: Mitali Borkar
> > > > > ---
> > > > >
> > > > > Changes from v1:- Reduced length of the line under 80 characters
> > > > >
> > > > > drivers/staging/media/ipu3/include/intel-ipu3.h | 3 ++-
> > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > > b/drivers/staging/media/ipu3/include/intel-ipu3.h index
> > > > > 6a72c81d2b67..52dcc6cdcffc 100644
> > > > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > > @@ -247,7 +247,8 @@ struct ipu3_uapi_ae_ccm {
> > > > >
> > > > > */
> > > > >
> > > > > struct ipu3_uapi_ae_config {
> > > > >
> > > > > struct ipu3_uapi_ae_grid_config grid_cfg __aligned(32);
> > > > >
> > > > > - struct ipu3_uapi_ae_weight_elem weights[IPU3_UAPI_AE_WEIGHTS]
> > > > > __aligned(32); + struct ipu3_uapi_ae_weight_elem
> > > > > weights[IPU3_UAPI_AE_WEIGHTS] +
__aligned(32);
> > > >
> > > > Do you still have the other two patches in your tree? This doesn't
> > > > apply
> > > > here due to the different attribute syntax.
> > >
> > > I have patch 1/6 and 2/6 in my tree which you asked me to drop.
> >
> > Could you drop them and then submit v3?
>
> I am extremely sorry Sir, but I am still learning to use git, drop them
> means to delete those commits? Even if I delete those, this patch was
> made after those, so the changes I made then will remain as it is, so
> what to do now?
>
> > Thanks.
>
I think that this document can help:
https://kernelnewbies.org/GitTips
Fabio
[Outreachy kernel] [PATCH] staging: rtl8192u: ieee80211: Replaced strncpy() with strscpy()
Replaced strncpy() with strscpy() because of compilation time warnings
about possible truncation of output [-Wstringop-truncation].
Furthermore, according to the Linux official documentation, strscpy() is
preferred to strncpy.
Signed-off-by: Fabio M. De Francesco
---
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
index 25ea8e1b6b65..aa58eedf5e86 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
@@ -1385,12 +1385,12 @@ inline void ieee80211_softmac_new_net(struct
ieee80211_device *ieee, struct ieee
* essid provided by the user.
*/
if (!ssidbroad) {
- strncpy(tmp_ssid, ieee->current_network.ssid,
IW_ESSID_MAX_SIZE);
+ strscpy(tmp_ssid, ieee->current_network.ssid,
IW_ESSID_MAX_SIZE);
tmp_ssid_len = ieee->current_network.ssid_len;
}
memcpy(&ieee->current_network, net, sizeof(struct
ieee80211_network));
- strncpy(ieee->current_network.ssid, tmp_ssid,
IW_ESSID_MAX_SIZE);
+ strscpy(ieee->current_network.ssid, tmp_ssid,
IW_ESSID_MAX_SIZE);
ieee->current_network.ssid_len = tmp_ssid_len;
netdev_info(ieee->dev,
"Linking with %s,channel:%d, qos:%d,
myHT:%d, networkHT:%d\n",
--
2.31.1
[Outreachy patch] [PATCH] staging: rtl8188eu: Move channel_table away from rtw_mlme_ext.h
Moved "static const struct channel_table[]" from include/rtw_mlme_ext.h
to core/rtw_ioctl_set.c because the latter is the only file that uses
that array of struct(s) in the whole driver. "make rtl8188eu/ W=1" output
several warnings about "'channel_table' defined but not used
[-Wunused-const-variable=]".
Signed-off-by: Fabio M. De Francesco
---
drivers/staging/rtl8188eu/core/rtw_ioctl_set.c | 8
drivers/staging/rtl8188eu/include/rtw_mlme_ext.h | 8
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_ioctl_set.c
b/drivers/staging/rtl8188eu/core/rtw_ioctl_set.c
index 1ef32ff900a9..17b999f45132 100644
--- a/drivers/staging/rtl8188eu/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8188eu/core/rtw_ioctl_set.c
@@ -11,6 +11,14 @@
#include
#include
+static const struct {
+int channel_plan;
+char *name;
+} channel_table[] = { { RT_CHANNEL_DOMAIN_FCC, "US" },
+{ RT_CHANNEL_DOMAIN_ETSI, "EU" },
+{ RT_CHANNEL_DOMAIN_MKK, "JP" },
+{ RT_CHANNEL_DOMAIN_CHINA, "CN"} };
+
extern void indicate_wx_scan_complete_event(struct adapter *padapter);
u8 rtw_do_join(struct adapter *padapter)
diff --git a/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
b/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
index 77eb5e3ef172..03d55eb7dc16 100644
--- a/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
@@ -171,14 +171,6 @@ struct rt_channel_plan_map {
unsigned char Index2G;
};
-static const struct {
- int channel_plan;
- char *name;
-} channel_table[] = { { RT_CHANNEL_DOMAIN_FCC, "US" },
- { RT_CHANNEL_DOMAIN_ETSI, "EU" },
- { RT_CHANNEL_DOMAIN_MKK, "JP" },
- { RT_CHANNEL_DOMAIN_CHINA, "CN"} };
-
enum Associated_AP {
atherosAP = 0,
broadcomAP = 1,
--
2.31.1
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: hal: Remove camelcase
On Tuesday, April 13, 2021 9:39:44 AM CEST Greg Kroah-Hartman wrote: > On Mon, Apr 12, 2021 at 11:02:58PM +0200, Fabio M. De Francesco wrote: > > Removed camelcase in (some) symbols. Further work is needed. > > What symbols did you do this for? What did you change them from and to?> > > Be specific, and try to do only one structure at a time at the most, > trying to review 1000 lines of changes at once is hard, would you want > to do that? :) > For sure, if I were you, I wouldn't be reviewing one thousand lines of code at once :) OK, let's work on one item at a time I'd place each change in a patch of a series: a) one global variable -> one patch with subject containing the name of the symbol (beware that if not 1000 lines you'll have to review 200 lines or more because that file is huge and its globals are used hundreds of times); b) one or more local variables -> one single patch containing the name of the function where they are used; In the while, I'll remove also the trailing "p_" that stand for "pointer" (as Julia suggested). The above seems reasonable to me. Doesn't it? Regarding what Matthew wrote about making use of drivers/net/wireless/realtek/rtlwifi/btcoexist/ I cannot work on it (too much time commitment would be needed, I suppose), but, if someone else can, I won't do the above-mentioned tasks because they would be useless. Said that, I'm not sure if removing camelcase in this file should be done at all. Thanks, Fabio > > thanks, > > greg k-h
Re: [Outreachy kernel] [PATCH 0/2] Remove spaces and blank lines
On Mon, Apr 12, 2021 at 07:59:01PM +0200, Fabio M. De Francesco wrote: > Removed spaces before tabs and multiple blank lines from Hal8723BReg.h > for readability improvement. Issues detected by checkpatch.pl. > > Fabio M. De Francesco (2): > staging: rtl8723bs: hal Remove spaces before tabs > staging: rtl8723bs: hal: Remove multiple blank lines > > drivers/staging/rtl8723bs/hal/Hal8723BReg.h | 43 + > 1 file changed, 18 insertions(+), 25 deletions(-) This series does not apply to my tree, please rebase and resend. thanks, greg k-h
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: hal: Remove camelcase
On Mon, Apr 12, 2021 at 11:02:58PM +0200, Fabio M. De Francesco wrote: > Removed camelcase in (some) symbols. Further work is needed. What symbols did you do this for? What did you change them from and to? Be specific, and try to do only one structure at a time at the most, trying to review 1000 lines of changes at once is hard, would you want to do that? :) thanks, greg k-h
Re: [Outreachy kernel] Subject: [PATCH v2] staging: media: meson: vdec: declare u32 as static const appropriately
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 4/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss_resizer.c
Hi Aline,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
url:
https://github.com/0day-ci/linux/commits/Aline-Santana-Cordeiro/staging-media-omap4iss-Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-215756
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
f2f560e1bdc055a6a306e6b7823ba589794e6564
config: mips-randconfig-r013-20210412 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
9829f5e6b1bca9b61efc629770d28bb9014dec45)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
#
https://github.com/0day-ci/linux/commit/82d48fcd5db8ca859bb928988c9b85d32e1eadbd
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Aline-Santana-Cordeiro/staging-media-omap4iss-Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-215756
git checkout 82d48fcd5db8ca859bb928988c9b85d32e1eadbd
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/staging/media/omap4iss/iss_resizer.c:35:15: error: member reference
type 'int' is not a pointer
dev_dbg(iss->dev, "###RSZ " #name "=0x%08x\n",
~~~ ^
include/linux/dev_printk.h:131:26: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^~~
drivers/staging/media/omap4iss/iss_resizer.c:35:30: error: expected ')'
dev_dbg(iss->dev, "###RSZ " #name "=0x%08x\n",
^
drivers/staging/media/omap4iss/iss_resizer.c:35:2: note: to match this '('
dev_dbg(iss->dev, "###RSZ " #name "=0x%08x\n",
^
include/linux/dev_printk.h:131:13: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^
drivers/staging/media/omap4iss/iss_resizer.c:41:15: error: member reference
type 'int' is not a pointer
dev_dbg(iss->dev, "###RZA " #name "=0x%08x\n",
~~~ ^
include/linux/dev_printk.h:131:26: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^~~
drivers/staging/media/omap4iss/iss_resizer.c:41:30: error: expected ')'
dev_dbg(iss->dev, "###RZA " #name "=0x%08x\n",
^
drivers/staging/media/omap4iss/iss_resizer.c:41:2: note: to match this '('
dev_dbg(iss->dev, "###RZA " #name "=0x%08x\n",
^
include/linux/dev_printk.h:131:13: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^
drivers/staging/media/omap4iss/iss_resizer.c:51:26: error: use of undeclared
identifier 'SYSCONFIG'
rsz_print_register(iss, SYSCONFIG);
^
drivers/staging/media/omap4iss/iss_resizer.c:52:26: error: use of undeclared
identifier 'IN_FIFO_CTRL'
rsz_print_register(iss, IN_FIFO_CTRL);
^
drivers/staging/media/omap4iss/iss_resizer.c:53:26: error: use of undeclared
identifier 'FRACDIV'
rsz_print_register(iss, FRACDIV);
^
drivers/staging/media/omap4iss/iss_resizer.c:54:26: error: use of undeclared
identifier 'SRC_EN'
rsz_print_register(iss, SRC_EN);
^
drivers/staging/media/omap4iss/iss_resizer.c:55:26: error: use of undeclared
identifier 'SRC_MODE'
rsz_print_register(iss, SRC_MODE);
^
drivers/staging/media/omap4iss/iss_resizer.c:56:26: error: use of undeclared
identifier 'SRC_FMT0'
rsz_print_register(iss, SRC_FMT0);
^
drivers/staging/media/omap4iss/iss_resizer.c:57:26: error: use of undeclared
identifier 'SRC_FMT1'
rsz_print_register(iss, SRC_FMT1);
^
drivers/staging/media/omap4iss/iss_resizer.c:58:26: error: use of undeclared
identifier 'SRC_VPS'
rsz_print_register(iss, SRC_VPS);
^
drivers/staging/media/omap4iss/iss_resizer.c:59:26: error: use of undeclared
identifier 'SRC_VSZ'
rsz_print_register(iss, SRC_VSZ);
^
dri
Re: [Outreachy kernel][PATCH 3/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss_ipipeif.c
Hi Aline,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
url:
https://github.com/0day-ci/linux/commits/Aline-Santana-Cordeiro/staging-media-omap4iss-Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-215756
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
f2f560e1bdc055a6a306e6b7823ba589794e6564
config: mips-randconfig-r013-20210412 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
9829f5e6b1bca9b61efc629770d28bb9014dec45)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
#
https://github.com/0day-ci/linux/commit/227d208756f87cbe3c143fa2a3a8f91103c6858e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Aline-Santana-Cordeiro/staging-media-omap4iss-Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-215756
git checkout 227d208756f87cbe3c143fa2a3a8f91103c6858e
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/staging/media/omap4iss/iss_ipipeif.c:39:15: error: member reference
type 'int' is not a pointer
dev_dbg(iss->dev, "###IPIPEIF " #name "=0x%08x\n",
~~~ ^
include/linux/dev_printk.h:131:26: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^~~
drivers/staging/media/omap4iss/iss_ipipeif.c:39:34: error: expected ')'
dev_dbg(iss->dev, "###IPIPEIF " #name "=0x%08x\n",
^
drivers/staging/media/omap4iss/iss_ipipeif.c:39:2: note: to match this '('
dev_dbg(iss->dev, "###IPIPEIF " #name "=0x%08x\n",
^
include/linux/dev_printk.h:131:13: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^
drivers/staging/media/omap4iss/iss_ipipeif.c:45:15: error: member reference
type 'int' is not a pointer
dev_dbg(iss->dev, "###ISIF " #name "=0x%08x\n",
~~~ ^
include/linux/dev_printk.h:131:26: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^~~
drivers/staging/media/omap4iss/iss_ipipeif.c:45:31: error: expected ')'
dev_dbg(iss->dev, "###ISIF " #name "=0x%08x\n",
^
drivers/staging/media/omap4iss/iss_ipipeif.c:45:2: note: to match this '('
dev_dbg(iss->dev, "###ISIF " #name "=0x%08x\n",
^
include/linux/dev_printk.h:131:13: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^
drivers/staging/media/omap4iss/iss_ipipeif.c:51:15: error: member reference
type 'int' is not a pointer
dev_dbg(iss->dev, "###ISP5 " #name "=0x%08x\n",
~~~ ^
include/linux/dev_printk.h:131:26: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^~~
drivers/staging/media/omap4iss/iss_ipipeif.c:51:31: error: expected ')'
dev_dbg(iss->dev, "###ISP5 " #name "=0x%08x\n",
^
drivers/staging/media/omap4iss/iss_ipipeif.c:51:2: note: to match this '('
dev_dbg(iss->dev, "###ISP5 " #name "=0x%08x\n",
^
include/linux/dev_printk.h:131:13: note: expanded from macro 'dev_dbg'
dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
^
drivers/staging/media/omap4iss/iss_ipipeif.c:61:30: error: use of undeclared
identifier 'CFG1'
ipipeif_print_register(iss, CFG1);
^
drivers/staging/media/omap4iss/iss_ipipeif.c:62:30: error: use of undeclared
identifier 'CFG2'
ipipeif_print_register(iss, CFG2);
^
drivers/staging/media/omap4iss/iss_ipipeif.c:64:27: error: use of undeclared
identifier 'SYNCEN'
isif_print_register(iss, SYNCEN);
^
drivers/staging/media/omap4iss/iss_ipipeif.c:65:27: error: use of undeclared
identifier 'CADU'
isif_print_register(iss, CADU);
^
drivers/staging/med
Re: [Outreachy kernel] Re: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)
Hi Mitali, On Mon, Apr 12, 2021 at 07:59:29PM +0530, Mitali Borkar wrote: > On Mon, Apr 12, 2021 at 12:43:15PM +0300, Sakari Ailus wrote: > > Hi Mitali, > > > > On Mon, Apr 12, 2021 at 04:38:59AM +0530, Mitali Borkar wrote: > > > This patch fixes the warning identified by checkpatch.pl by replacing > > > __attribute__aligned(size) with __aligned(size) > > > > Same comments on this than the 1st patch. > > > > It's a staging driver so even if this is a user space header, it's not > > under include/uapi/linux. > > > Sir, I am not able to understandd what you are trying to say in this. As you > mentioned in patch 1/6, I removed and added header where BIT() macro under > apprpriate userpace, but what should I modify in this patch? The comment on the 1st patch and above was a weird way of saying "please drop patches 1 and 2". BIT(), __aligned() and __packed are macros in kernel headers that generally are not available in headers exported for user space consumption. -- Kind regards, Sakari Ailus
Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: hal: Remove camelcase
On Mon, Apr 12, 2021 at 11:02:58PM +0200, Fabio M. De Francesco wrote: > Removed camelcase in (some) symbols. Further work is needed. What would be more useful for this driver is making it use drivers/net/wireless/realtek/rtlwifi/btcoexist/ which has already graduated out of staging. I haven't checked how closely it matches, but this is surely a better use of time than "cleaning up" this version.
Re: [Outreachy kernel][PATCH 3/4] Replace macro function by static inline function in file iss_ipipeif.c
Hi Aline, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v5.12-rc7 next-20210412] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Aline-Santana-Cordeiro/Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-213252 base: git://linuxtv.org/media_tree.git master config: arc-allyesconfig (attached as .config) compiler: arceb-elf-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/8845073e7a4b434b2bc32818bfa172e91075a167 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Aline-Santana-Cordeiro/Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-213252 git checkout 8845073e7a4b434b2bc32818bfa172e91075a167 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): drivers/staging/media/omap4iss/iss_ipipeif.c:37:15: error: return type defaults to 'int' [-Werror=return-type] 37 | static inline ipipeif_print_register(iss, name) | ^~ drivers/staging/media/omap4iss/iss_ipipeif.c:37:15: error: function declaration isn't a prototype [-Werror=strict-prototypes] drivers/staging/media/omap4iss/iss_ipipeif.c: In function 'ipipeif_print_register': >> drivers/staging/media/omap4iss/iss_ipipeif.c:37:15: warning: old-style >> function definition [-Wold-style-definition] >> drivers/staging/media/omap4iss/iss_ipipeif.c:37:15: warning: type of 'iss' >> defaults to 'int' [-Wmissing-parameter-type] >> drivers/staging/media/omap4iss/iss_ipipeif.c:37:15: warning: type of 'name' >> defaults to 'int' [-Wmissing-parameter-type] In file included from include/linux/printk.h:409, from include/linux/kernel.h:16, from include/linux/list.h:9, from include/linux/module.h:12, from drivers/staging/media/omap4iss/iss_ipipeif.c:10: drivers/staging/media/omap4iss/iss_ipipeif.c:39:34: error: stray '#' in program 39 | dev_dbg(iss->dev, "###IPIPEIF " #name "=0x%08x\n", | ^ include/linux/dynamic_debug.h:91:14: note: in definition of macro 'DEFINE_DYNAMIC_DEBUG_METADATA' 91 | .format = (fmt),\ | ^~~ include/linux/dynamic_debug.h:147:2: note: in expansion of macro '__dynamic_func_call' 147 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__) | ^~~ include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call' 161 | _dynamic_func_call(fmt,__dynamic_dev_dbg, \ | ^~ include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg' 123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt' 123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ drivers/staging/media/omap4iss/iss_ipipeif.c:39:2: note: in expansion of macro 'dev_dbg' 39 | dev_dbg(iss->dev, "###IPIPEIF " #name "=0x%08x\n", | ^~~ drivers/staging/media/omap4iss/iss_ipipeif.c:39:35: error: expected ')' before 'name' 39 | dev_dbg(iss->dev, "###IPIPEIF " #name "=0x%08x\n", | ^~~~ include/linux/dynamic_debug.h:91:14: note: in definition of macro 'DEFINE_DYNAMIC_DEBUG_METADATA' 91 | .format = (fmt),\ | ^~~ include/linux/dynamic_debug.h:147:2: note: in expansion of macro '__dynamic_func_call' 147 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__) | ^~~ include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call' 161 | _dynamic_func_call(fmt,__dynamic_dev_dbg, \ | ^~ include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg' 123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt' 123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ drivers/staging/media/omap4iss/iss_ipipeif.c:39:2: note: in expansion of
Re: [Outreachy kernel][PATCH 2/4] Replace macro function by static inline function in file iss_ipipe.c
Hi Aline, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v5.12-rc7 next-20210412] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Aline-Santana-Cordeiro/Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-213252 base: git://linuxtv.org/media_tree.git master config: arc-allyesconfig (attached as .config) compiler: arceb-elf-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/be8ef400d02afa928839bd6264ba2100ac02986d git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Aline-Santana-Cordeiro/Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-213252 git checkout be8ef400d02afa928839bd6264ba2100ac02986d # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): drivers/staging/media/omap4iss/iss_ipipe.c:41:15: error: return type defaults to 'int' [-Werror=return-type] 41 | static inline ipipe_print_register(iss, name) | ^~~~ drivers/staging/media/omap4iss/iss_ipipe.c:41:15: error: function declaration isn't a prototype [-Werror=strict-prototypes] drivers/staging/media/omap4iss/iss_ipipe.c: In function 'ipipe_print_register': >> drivers/staging/media/omap4iss/iss_ipipe.c:41:15: warning: old-style >> function definition [-Wold-style-definition] >> drivers/staging/media/omap4iss/iss_ipipe.c:41:15: warning: type of 'iss' >> defaults to 'int' [-Wmissing-parameter-type] >> drivers/staging/media/omap4iss/iss_ipipe.c:41:15: warning: type of 'name' >> defaults to 'int' [-Wmissing-parameter-type] In file included from include/linux/printk.h:409, from include/linux/kernel.h:16, from include/linux/list.h:9, from include/linux/module.h:12, from drivers/staging/media/omap4iss/iss_ipipe.c:10: drivers/staging/media/omap4iss/iss_ipipe.c:43:32: error: stray '#' in program 43 | dev_dbg(iss->dev, "###IPIPE " #name "=0x%08x\n", |^ include/linux/dynamic_debug.h:91:14: note: in definition of macro 'DEFINE_DYNAMIC_DEBUG_METADATA' 91 | .format = (fmt),\ | ^~~ include/linux/dynamic_debug.h:147:2: note: in expansion of macro '__dynamic_func_call' 147 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__) | ^~~ include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call' 161 | _dynamic_func_call(fmt,__dynamic_dev_dbg, \ | ^~ include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg' 123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt' 123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ drivers/staging/media/omap4iss/iss_ipipe.c:43:2: note: in expansion of macro 'dev_dbg' 43 | dev_dbg(iss->dev, "###IPIPE " #name "=0x%08x\n", | ^~~ drivers/staging/media/omap4iss/iss_ipipe.c:43:33: error: expected ')' before 'name' 43 | dev_dbg(iss->dev, "###IPIPE " #name "=0x%08x\n", | ^~~~ include/linux/dynamic_debug.h:91:14: note: in definition of macro 'DEFINE_DYNAMIC_DEBUG_METADATA' 91 | .format = (fmt),\ | ^~~ include/linux/dynamic_debug.h:147:2: note: in expansion of macro '__dynamic_func_call' 147 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__) | ^~~ include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call' 161 | _dynamic_func_call(fmt,__dynamic_dev_dbg, \ | ^~ include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg' 123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt' 123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ drivers/staging/media/omap4iss/iss_ipipe.c:43:2: note: in expansion of macro 'dev_dbg' 43 | dev_dbg(i
[Outreachy kernel] [PATCH 2/2] staging: rtl8723bs: hal: Remove multiple blank lines
Removed unnecessary multiple blank lines. Issue detected by checkpatch.pl. Signed-off-by: Fabio M. De Francesco --- drivers/staging/rtl8723bs/hal/Hal8723BReg.h | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/staging/rtl8723bs/hal/Hal8723BReg.h b/drivers/staging/rtl8723bs/hal/Hal8723BReg.h index 1ff2043fb2e2..4a91913e20bf 100644 --- a/drivers/staging/rtl8723bs/hal/Hal8723BReg.h +++ b/drivers/staging/rtl8723bs/hal/Hal8723BReg.h @@ -19,12 +19,9 @@ #ifndef __INC_HAL8723BREG_H #define __INC_HAL8723BREG_H - - /* */ /* */ /* */ - /* */ /* */ /* 0xh ~ 0x00FFh System Configuration */ @@ -142,7 +139,6 @@ #define REG_RQPN_NPQ_8723B 0x0214 #define REG_DWBCN1_CTRL_8723B 0x0228 - /* */ /* */ /* 0x0280h ~ 0x02FFh RXDMA Configuration */ @@ -158,7 +154,6 @@ #define REG_RSVD5_8723B0x02F0 #define REG_RSVD6_8723B0x02F4 - /* */ /* */ /* 0x0300h ~ 0x03FFh PCIe */ @@ -355,7 +350,6 @@ #define REG_BFMEE_SEL_8723B0x0714 #define REG_SND_PTCL_CTRL_8723B0x0718 - /* Redifine 8192C register definition for compatibility */ /* TODO: use these definition when using REG_xxx naming rule. */ @@ -365,7 +359,6 @@ #defineMSR_8723B (REG_CR_8723B + 2) /* Media Status register */ #defineISR_8723B REG_HISR0_8723B #defineTSFR_8723B REG_TSFTR_8723B /* Timing Sync Function Timer Register. */ - #define PBP_8723B REG_PBP_8723B /* Redifine MACID register, to compatible prior ICs. */ -- 2.31.1
[Outreachy kernel] [PATCH 1/2] staging: rtl8723bs: hal: Remove spaces before tabs
Removed spaces before tabs. Issue detected by checkpatch.pl. Signed-off-by: Fabio M. De Francesco --- drivers/staging/rtl8723bs/hal/Hal8723BReg.h | 36 ++--- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/drivers/staging/rtl8723bs/hal/Hal8723BReg.h b/drivers/staging/rtl8723bs/hal/Hal8723BReg.h index 616d20106392..1ff2043fb2e2 100644 --- a/drivers/staging/rtl8723bs/hal/Hal8723BReg.h +++ b/drivers/staging/rtl8723bs/hal/Hal8723BReg.h @@ -27,7 +27,7 @@ /* */ /* */ -/* 0xh ~ 0x00FFh System Configuration */ +/* 0xh ~ 0x00FFh System Configuration */ /* */ /* */ #define REG_SYS_ISO_CTRL_8723B 0x /* 2 Byte */ @@ -84,7 +84,7 @@ /* */ /* */ -/* 0x0100h ~ 0x01FFh MACTOP General Configuration */ +/* 0x0100h ~ 0x01FFh MACTOP General Configuration */ /* */ /* */ #define REG_CR_8723B 0x0100 @@ -131,7 +131,7 @@ /* */ /* */ -/* 0x0200h ~ 0x027Fh TXDMA Configuration */ +/* 0x0200h ~ 0x027Fh TXDMA Configuration */ /* */ /* */ #define REG_RQPN_8723B 0x0200 @@ -145,7 +145,7 @@ /* */ /* */ -/* 0x0280h ~ 0x02FFh RXDMA Configuration */ +/* 0x0280h ~ 0x02FFh RXDMA Configuration */ /* */ /* */ #define REG_RXDMA_AGG_PG_TH_8723B 0x0280 @@ -161,7 +161,7 @@ /* */ /* */ -/* 0x0300h ~ 0x03FFh PCIe */ +/* 0x0300h ~ 0x03FFh PCIe */ /* */ /* */ #defineREG_PCIE_CTRL_REG_8723B 0x0300 @@ -189,7 +189,7 @@ /* spec version 11 */ /* */ /* */ -/* 0x0400h ~ 0x047Fh Protocol Configuration */ +/* 0x0400h ~ 0x047Fh Protocol Configuration */ /* */ /* */ #define REG_VOQ_INFORMATION_8723B 0x0400 @@ -243,7 +243,7 @@ /* */ /* */ -/* 0x0500h ~ 0x05FFh EDCA Configuration */ +/* 0x0500h ~ 0x05FFh EDCA Configuration */ /* */ /* */ #define REG_EDCA_VO_PARAM_8723B0x0500 @@ -263,15 +263,15 @@ #define REG_RD_CTRL_8723B 0x0524 /* */ /* Format for offset 540h-542h: */ -/* [3:0]: TBTT prohibit setup in unit of 32us. The time for HW getting beacon content before TBTT. */ -/* [7:4]: Reserved. */ -/* [19:8]: TBTT prohibit hold in unit of 32us. The time for HW holding to send the beacon packet. */ -/* [23:20]: Reserved */ +/* [3:0]: TBTT prohibit setup in unit of 32us. The time for HW getting beacon content before TBTT. */ +/* [7:4]: Reserved. */ +/* [19:8]: TBTT prohibit hold in unit of 32us. The time for HW holding to send the beacon packet. */ +/* [23:20]: Reserved */ /* Description: */ -/* | */ +/* | */ /* |<--Setup--|--Hold>| */ -/* --|-- */ -/* | */ +/* --|-- */ +/* | */ /*TBTT */ /* Note: We cannot update beacon content to HW or send any AC packets during the time between Setup and Hold. */ /* Described by Designer Tim and Bruce, 2011-01-14. */ @@ -300,7 +300,7 @@ #define REG_ACMHWCTRL_8723B0x05C0 #define REG_SCH_TXCMD_8723B0x05F8 -/* 0x0600h ~ 0x07FFh WMAC Configuration */ +/* 0x0600h ~ 0x07FFh WMAC Configuration */ #define REG_MAC_CR_8723B 0x0600 #define REG_TCR_8723B 0x0604 #define REG_RCR_8723B 0x0608 @@ -356,10 +356,10 @@ #define REG_SND_PTCL_CTRL_8723B0x0718 -/* Redifine 8192C register definition for compatibility */ +/* Redifine 8192C register definition for compatibility */ -/* TODO: use these definition when using REG_xxx naming rule. */ -/* NOTE: DO NOT Remove these definition. Use later. */ +/* TODO: use these definition when using REG_xxx naming rule. */ +/* NOTE: DO NOT Remove these definition. Use later. */ #defineEFUSE_CTRL_8723BREG_EFUSE_CTRL_8723B/* E-Fuse Control. */ #defineEFUSE_TEST_8723BREG_EFUSE_TEST_8723B/* E-Fuse Test. */ #defineMSR_8723B (REG_CR_8723B + 2) /* Media Status register */ -- 2.31.1
[Outreachy kernel] [PATCH 0/2] Remove spaces and blank lines
Removed spaces before tabs and multiple blank lines from Hal8723BReg.h for readability improvement. Issues detected by checkpatch.pl. Fabio M. De Francesco (2): staging: rtl8723bs: hal Remove spaces before tabs staging: rtl8723bs: hal: Remove multiple blank lines drivers/staging/rtl8723bs/hal/Hal8723BReg.h | 43 + 1 file changed, 18 insertions(+), 25 deletions(-) -- 2.31.1
Re: [Outreachy kernel][PATCH 1/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss.c
Em seg, 2021-04-12 às 19:36 +0200, Julia Lawall escreveu: > > > 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 Ok! I'll do that from now on. Thank you for the help, Aline
Re: [Outreachy kernel][PATCH 1/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss.c
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][PATCH 1/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss.c
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. Thank you for warning me, Aline
Re: [Outreachy kernel][PATCH 1/4] Replace macro function by static inline function in file iss.c
Hi Aline,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.12-rc7 next-20210412]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Aline-Santana-Cordeiro/Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-213252
base: git://linuxtv.org/media_tree.git master
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/31032f2aa94de36a70de87f425eac62a2000367d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Aline-Santana-Cordeiro/Replace-macro-function-by-static-inline-function-in-file-iss-c/20210412-213252
git checkout 31032f2aa94de36a70de87f425eac62a2000367d
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
drivers/staging/media/omap4iss/iss.c:30:15: error: return type defaults to
'int' [-Werror=return-type]
30 | static inline iss_print_register(iss, name)
| ^~
drivers/staging/media/omap4iss/iss.c:30:15: error: function declaration
isn't a prototype [-Werror=strict-prototypes]
drivers/staging/media/omap4iss/iss.c: In function 'iss_print_register':
>> drivers/staging/media/omap4iss/iss.c:30:15: warning: old-style function
>> definition [-Wold-style-definition]
>> drivers/staging/media/omap4iss/iss.c:30:15: warning: type of 'iss' defaults
>> to 'int' [-Wmissing-parameter-type]
>> drivers/staging/media/omap4iss/iss.c:30:15: warning: type of 'name' defaults
>> to 'int' [-Wmissing-parameter-type]
In file included from include/linux/printk.h:409,
from include/linux/kernel.h:16,
from include/linux/clk.h:13,
from drivers/staging/media/omap4iss/iss.c:10:
drivers/staging/media/omap4iss/iss.c:32:30: error: stray '#' in program
32 | dev_dbg(iss->dev, "###ISS " #name "=0x%08x\n",
| ^
include/linux/dynamic_debug.h:91:14: note: in definition of macro
'DEFINE_DYNAMIC_DEBUG_METADATA'
91 | .format = (fmt),\
| ^~~
include/linux/dynamic_debug.h:147:2: note: in expansion of macro
'__dynamic_func_call'
147 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:161:2: note: in expansion of macro
'_dynamic_func_call'
161 | _dynamic_func_call(fmt,__dynamic_dev_dbg, \
| ^~
include/linux/dev_printk.h:123:2: note: in expansion of macro
'dynamic_dev_dbg'
123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
drivers/staging/media/omap4iss/iss.c:32:2: note: in expansion of macro
'dev_dbg'
32 | dev_dbg(iss->dev, "###ISS " #name "=0x%08x\n",
| ^~~
drivers/staging/media/omap4iss/iss.c:32:31: error: expected ')' before 'name'
32 | dev_dbg(iss->dev, "###ISS " #name "=0x%08x\n",
| ^~~~
include/linux/dynamic_debug.h:91:14: note: in definition of macro
'DEFINE_DYNAMIC_DEBUG_METADATA'
91 | .format = (fmt),\
| ^~~
include/linux/dynamic_debug.h:147:2: note: in expansion of macro
'__dynamic_func_call'
147 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:161:2: note: in expansion of macro
'_dynamic_func_call'
161 | _dynamic_func_call(fmt,__dynamic_dev_dbg, \
| ^~
include/linux/dev_printk.h:123:2: note: in expansion of macro
'dynamic_dev_dbg'
123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
123 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
drivers/staging/media/omap4iss/iss.c:32:2: note: in expansion of macro
'dev_dbg'
32 | dev_dbg(iss->dev, "###ISS " #name "=0x%08x\n",
| ^~~
include/linux/dynamic_debug.h:91:13: note: to match this '('
91 | .
Re: [Outreachy kernel][PATCH 1/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss.c
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 ? -- Regards, Laurent Pinchart
Re: [Outreachy kernel] Re: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)
On Mon, Apr 12, 2021 at 12:43:15PM +0300, Sakari Ailus wrote: > Hi Mitali, > > On Mon, Apr 12, 2021 at 04:38:59AM +0530, Mitali Borkar wrote: > > This patch fixes the warning identified by checkpatch.pl by replacing > > __attribute__aligned(size) with __aligned(size) > > Same comments on this than the 1st patch. > > It's a staging driver so even if this is a user space header, it's not > under include/uapi/linux. > Sir, I am not able to understandd what you are trying to say in this. As you mentioned in patch 1/6, I removed and added header where BIT() macro under apprpriate userpace, but what should I modify in this patch? > -- > Kind regards, > > Sakari Ailus > > -- > 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/20210412094315.GJ3%40paasikivi.fi.intel.com.
Re: [Outreachy kernel][PATCH 1/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss.c
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. Thank you for the feedback. Aline
