| Message ID | 20201118220357.22292-6-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: make ksz8795 driver more dynamic | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 0 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 41 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 0 this patch: 7 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, Nov 18, 2020 at 11:03:51PM +0100, Michael Grzeschik wrote: > The variable mib_cnt is assigned with TOTAL_SWITCH_COUNTER_NUM. This > value can also be derived from the array size of mib_names. This patch > uses this calculated value instead, removes the extra define and uses > mib_cnt everywhere possible instead of the static define > TOTAL_SWITCH_COUNTER_NUM. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/net/dsa/microchip/ksz8795.c | 8 ++++---- > drivers/net/dsa/microchip/ksz8795_reg.h | 3 --- > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c > index 04a571bde7e6a4f..6ddba2de2d3026e 100644 > --- a/drivers/net/dsa/microchip/ksz8795.c > +++ b/drivers/net/dsa/microchip/ksz8795.c > @@ -23,7 +23,7 @@ > > static const struct { > char string[ETH_GSTRING_LEN]; > -} mib_names[TOTAL_SWITCH_COUNTER_NUM] = { > +} mib_names[] = { > { "rx_hi" }, > { "rx_undersize" }, > { "rx_fragments" }, > @@ -656,7 +656,7 @@ static void ksz8795_get_strings(struct dsa_switch *ds, int port, > { > int i; > > - for (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) { > + for (i = 0; i < dev->mib_cnt; i++) { > memcpy(buf + i * ETH_GSTRING_LEN, mib_names[i].string, > ETH_GSTRING_LEN); > } > @@ -1236,7 +1236,7 @@ static int ksz8795_switch_init(struct ksz_device *dev) > dev->port_mask |= dev->host_mask; > > dev->reg_mib_cnt = KSZ8795_COUNTER_NUM; > - dev->mib_cnt = TOTAL_SWITCH_COUNTER_NUM; > + dev->mib_cnt = ARRAY_SIZE(mib_names); Hi Michael I think it would be better to just use ARRAY_SIZE(mib_names) everywhere. It is one less hoop to jump through when looking for array overruns, etc. Andrew
On 11/18/2020 2:03 PM, Michael Grzeschik wrote: > The variable mib_cnt is assigned with TOTAL_SWITCH_COUNTER_NUM. This > value can also be derived from the array size of mib_names. This patch > uses this calculated value instead, removes the extra define and uses > mib_cnt everywhere possible instead of the static define > TOTAL_SWITCH_COUNTER_NUM. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on net-next/master ipvs/master linus/master v5.10-rc4 next-20201118]
[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/Michael-Grzeschik/net-dsa-microchip-make-ksz8795-driver-more-dynamic/20201119-060705
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git a3dcb3e7e70c72a68a79b30fc3a3adad5612731c
config: h8300-randconfig-r031-20201119 (attached as .config)
compiler: h8300-linux-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/9debc1a2179402cc4391e253c57fafa0ef357e05
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Michael-Grzeschik/net-dsa-microchip-make-ksz8795-driver-more-dynamic/20201119-060705
git checkout 9debc1a2179402cc4391e253c57fafa0ef357e05
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=h8300
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/net/dsa/microchip/ksz8795.c: In function 'ksz8795_get_strings':
>> drivers/net/dsa/microchip/ksz8795.c:659:18: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
659 | for (i = 0; i < dev->mib_cnt; i++) {
| ^~~
| cdev
drivers/net/dsa/microchip/ksz8795.c:659:18: note: each undeclared identifier is reported only once for each function it appears in
vim +659 drivers/net/dsa/microchip/ksz8795.c
653
654 static void ksz8795_get_strings(struct dsa_switch *ds, int port,
655 u32 stringset, uint8_t *buf)
656 {
657 int i;
658
> 659 for (i = 0; i < dev->mib_cnt; i++) {
660 memcpy(buf + i * ETH_GSTRING_LEN, mib_names[i].string,
661 ETH_GSTRING_LEN);
662 }
663 }
664
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Andrew! On Thu, Nov 19, 2020 at 01:20:47AM +0100, Andrew Lunn wrote: >On Wed, Nov 18, 2020 at 11:03:51PM +0100, Michael Grzeschik wrote: >> The variable mib_cnt is assigned with TOTAL_SWITCH_COUNTER_NUM. This >> value can also be derived from the array size of mib_names. This patch >> uses this calculated value instead, removes the extra define and uses >> mib_cnt everywhere possible instead of the static define >> TOTAL_SWITCH_COUNTER_NUM. >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> --- >> drivers/net/dsa/microchip/ksz8795.c | 8 ++++---- >> drivers/net/dsa/microchip/ksz8795_reg.h | 3 --- >> 2 files changed, 4 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c >> index 04a571bde7e6a4f..6ddba2de2d3026e 100644 >> --- a/drivers/net/dsa/microchip/ksz8795.c >> +++ b/drivers/net/dsa/microchip/ksz8795.c >> @@ -23,7 +23,7 @@ >> >> static const struct { >> char string[ETH_GSTRING_LEN]; >> -} mib_names[TOTAL_SWITCH_COUNTER_NUM] = { >> +} mib_names[] = { >> { "rx_hi" }, >> { "rx_undersize" }, >> { "rx_fragments" }, >> @@ -656,7 +656,7 @@ static void ksz8795_get_strings(struct dsa_switch *ds, int port, >> { >> int i; >> >> - for (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) { >> + for (i = 0; i < dev->mib_cnt; i++) { >> memcpy(buf + i * ETH_GSTRING_LEN, mib_names[i].string, >> ETH_GSTRING_LEN); >> } >> @@ -1236,7 +1236,7 @@ static int ksz8795_switch_init(struct ksz_device *dev) >> dev->port_mask |= dev->host_mask; >> >> dev->reg_mib_cnt = KSZ8795_COUNTER_NUM; >> - dev->mib_cnt = TOTAL_SWITCH_COUNTER_NUM; >> + dev->mib_cnt = ARRAY_SIZE(mib_names); > >Hi Michael > >I think it would be better to just use ARRAY_SIZE(mib_names) >everywhere. It is one less hoop to jump through when looking for array >overruns, etc. I would better stay with the variable, because of two reasons. First it is also used in ksz_common.c and ksz_9477.c. Also in my next patches I will introduce another mib_names array. We will have ksz8863_mib_names and ksz8795_mib_names. In the init function then the mib_cnt will be set regarding to the chip that is found. Do you agree that the extra variable makes the code more readable in that case? Regards, Michael
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 04a571bde7e6a4f..6ddba2de2d3026e 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -23,7 +23,7 @@ static const struct { char string[ETH_GSTRING_LEN]; -} mib_names[TOTAL_SWITCH_COUNTER_NUM] = { +} mib_names[] = { { "rx_hi" }, { "rx_undersize" }, { "rx_fragments" }, @@ -656,7 +656,7 @@ static void ksz8795_get_strings(struct dsa_switch *ds, int port, { int i; - for (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) { + for (i = 0; i < dev->mib_cnt; i++) { memcpy(buf + i * ETH_GSTRING_LEN, mib_names[i].string, ETH_GSTRING_LEN); } @@ -1236,7 +1236,7 @@ static int ksz8795_switch_init(struct ksz_device *dev) dev->port_mask |= dev->host_mask; dev->reg_mib_cnt = KSZ8795_COUNTER_NUM; - dev->mib_cnt = TOTAL_SWITCH_COUNTER_NUM; + dev->mib_cnt = ARRAY_SIZE(mib_names); dev->mib_port_cnt = TOTAL_PORT_NUM; dev->phy_port_cnt = SWITCH_PORT_NUM; @@ -1254,7 +1254,7 @@ static int ksz8795_switch_init(struct ksz_device *dev) dev->ports[i].mib.counters = devm_kzalloc(dev->dev, sizeof(u64) * - (TOTAL_SWITCH_COUNTER_NUM + 1), + (dev->mib_cnt + 1), GFP_KERNEL); if (!dev->ports[i].mib.counters) return -ENOMEM; diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h index 25840719b936650..c131224850135bd 100644 --- a/drivers/net/dsa/microchip/ksz8795_reg.h +++ b/drivers/net/dsa/microchip/ksz8795_reg.h @@ -852,9 +852,6 @@ #define SWITCH_PORT_NUM (TOTAL_PORT_NUM - 1) #define KSZ8795_COUNTER_NUM 0x20 -#define TOTAL_KSZ8795_COUNTER_NUM (KSZ8795_COUNTER_NUM + 4) - -#define TOTAL_SWITCH_COUNTER_NUM TOTAL_KSZ8795_COUNTER_NUM /* Common names used by other drivers */
The variable mib_cnt is assigned with TOTAL_SWITCH_COUNTER_NUM. This value can also be derived from the array size of mib_names. This patch uses this calculated value instead, removes the extra define and uses mib_cnt everywhere possible instead of the static define TOTAL_SWITCH_COUNTER_NUM. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/net/dsa/microchip/ksz8795.c | 8 ++++---- drivers/net/dsa/microchip/ksz8795_reg.h | 3 --- 2 files changed, 4 insertions(+), 7 deletions(-)