| Message ID | 20240923053900.1310-1-kdipendra88@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: Add error pointer check in bcmsysport.c | expand |
On Mon, Sep 23, 2024 at 05:38:58AM +0000, Dipendra Khadka wrote: > Add error pointer checks in bcm_sysport_map_queues() and > bcm_sysport_unmap_queues() before deferencing 'dp'. nit: dereferencing Flagged by checkpatch.pl --codespell > > Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com> This patch does not compile. Please take care to make sure your paches compile. And, moroever, please slow down a bit. Please take some time to learn the process by getting one patch accepted. Rather going through that process with several patches simultaneously. > --- > v2: > - Change the subject of the patch to net I'm sorry to say that the subject is still not correct. Looking over the git history for this file, I would go for a prefix of 'net: systemport: '. I would also pass on mentioning the filename in the subject. Maybe: Subject: [PATCH v3 net] net: systemport: correct error pointer handling Also, I think that it would be better, although more verbose, to update these functions so that the assignment of dp occurs just before it is checked. In the case of bcm_sysport_map_queues(), that would look something like this (completely untested!): diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index c9faa8540859..7411f69a8806 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -2331,11 +2331,15 @@ static const struct net_device_ops bcm_sysport_netdev_ops = { static int bcm_sysport_map_queues(struct net_device *dev, struct net_device *slave_dev) { - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); struct bcm_sysport_priv *priv = netdev_priv(dev); struct bcm_sysport_tx_ring *ring; unsigned int num_tx_queues; unsigned int q, qp, port; + struct dsa_port *dp; + + dp = dsa_port_from_netdev(slave_dev); + if (IS_ERR(dp)) + return PTR_ERR(dp); /* We can't be setting up queue inspection for non directly attached * switches This patch is now targeted at 'net'. Which means that you believe it is a bug fix. I'd say that is reasonable, though it does seem to be somewhat theoretical. But in any case, a bug fix should have a Fixes tag, which describes the commit that added the bug. Alternatively, if it is not a bug fix, then it should be targeted at net-next (and not have a Fixes tag). Please note that net-next is currently closed for the v6.12 merge window. It shold re-open after v6.12-rc1 has been released, which I expect to occur about a week for now. You should wait for net-next to re-open before posting non-RFC patches for it. Lastly, when reposting patches, please note the 24h rule. https://docs.kernel.org/process/maintainer-netdev.html
Hi Simon, On Mon, 23 Sept 2024 at 22:04, Simon Horman <horms@kernel.org> wrote: > > On Mon, Sep 23, 2024 at 05:38:58AM +0000, Dipendra Khadka wrote: > > Add error pointer checks in bcm_sysport_map_queues() and > > bcm_sysport_unmap_queues() before deferencing 'dp'. > > nit: dereferencing > > Flagged by checkpatch.pl --codespell > > > > > Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com> > > This patch does not compile. > Please take care to make sure your paches compile. > > And, moroever, please slow down a bit. Please take some time to learn the > process by getting one patch accepted. Rather going through that process > with several patches simultaneously. > > > --- > > v2: > > - Change the subject of the patch to net > > I'm sorry to say that the subject is still not correct. > > Looking over the git history for this file, I would go for > a prefix of 'net: systemport: '. I would also pass on mentioning > the filename in the subject. Maybe: > > Subject: [PATCH v3 net] net: systemport: correct error pointer handling > > Also, I think that it would be better, although more verbose, > to update these functions so that the assignment of dp occurs > just before it is checked. > > In the case of bcm_sysport_map_queues(), that would look something like this > (completely untested!): > > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c > index c9faa8540859..7411f69a8806 100644 > --- a/drivers/net/ethernet/broadcom/bcmsysport.c > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c > @@ -2331,11 +2331,15 @@ static const struct net_device_ops bcm_sysport_netdev_ops = { > static int bcm_sysport_map_queues(struct net_device *dev, > struct net_device *slave_dev) > { > - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); > struct bcm_sysport_priv *priv = netdev_priv(dev); > struct bcm_sysport_tx_ring *ring; > unsigned int num_tx_queues; > unsigned int q, qp, port; > + struct dsa_port *dp; > + > + dp = dsa_port_from_netdev(slave_dev); > + if (IS_ERR(dp)) > + return PTR_ERR(dp); > > > /* We can't be setting up queue inspection for non directly attached > * switches > > > This patch is now targeted at 'net'. Which means that you believe > it is a bug fix. I'd say that is reasonable, though it does seem to > be somewhat theoretical. But in any case, a bug fix should > have a Fixes tag, which describes the commit that added the bug. > > Alternatively, if it is not a bug fix, then it should be targeted at > net-next (and not have a Fixes tag). Please note that net-next is currently > closed for the v6.12 merge window. It shold re-open after v6.12-rc1 has > been released, which I expect to occur about a week for now. You should > wait for net-next to re-open before posting non-RFC patches for it. > > Lastly, when reposting patches, please note the 24h rule. > https://docs.kernel.org/process/maintainer-netdev.html > Thank you so much for the response and the suggestions. I will follow everything you have said and whatever I have to. I was just hurrying to see my patch accepted. > -- > pw-bot: changes-requested > > Best Regards, Dipendra Khadka
On 9/23/24 09:39, Dipendra Khadka wrote: > Hi Simon, > > On Mon, 23 Sept 2024 at 22:04, Simon Horman <horms@kernel.org> wrote: >> >> On Mon, Sep 23, 2024 at 05:38:58AM +0000, Dipendra Khadka wrote: >>> Add error pointer checks in bcm_sysport_map_queues() and >>> bcm_sysport_unmap_queues() before deferencing 'dp'. >> >> nit: dereferencing >> >> Flagged by checkpatch.pl --codespell >> >>> >>> Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com> >> >> This patch does not compile. >> Please take care to make sure your paches compile. >> >> And, moroever, please slow down a bit. Please take some time to learn the >> process by getting one patch accepted. Rather going through that process >> with several patches simultaneously. >> >>> --- >>> v2: >>> - Change the subject of the patch to net >> >> I'm sorry to say that the subject is still not correct. >> >> Looking over the git history for this file, I would go for >> a prefix of 'net: systemport: '. I would also pass on mentioning >> the filename in the subject. Maybe: >> >> Subject: [PATCH v3 net] net: systemport: correct error pointer handling >> >> Also, I think that it would be better, although more verbose, >> to update these functions so that the assignment of dp occurs >> just before it is checked. >> >> In the case of bcm_sysport_map_queues(), that would look something like this >> (completely untested!): >> >> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c >> index c9faa8540859..7411f69a8806 100644 >> --- a/drivers/net/ethernet/broadcom/bcmsysport.c >> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c >> @@ -2331,11 +2331,15 @@ static const struct net_device_ops bcm_sysport_netdev_ops = { >> static int bcm_sysport_map_queues(struct net_device *dev, >> struct net_device *slave_dev) >> { >> - struct dsa_port *dp = dsa_port_from_netdev(slave_dev); >> struct bcm_sysport_priv *priv = netdev_priv(dev); >> struct bcm_sysport_tx_ring *ring; >> unsigned int num_tx_queues; >> unsigned int q, qp, port; >> + struct dsa_port *dp; >> + >> + dp = dsa_port_from_netdev(slave_dev); >> + if (IS_ERR(dp)) >> + return PTR_ERR(dp); >> >> >> /* We can't be setting up queue inspection for non directly attached >> * switches >> >> >> This patch is now targeted at 'net'. Which means that you believe >> it is a bug fix. I'd say that is reasonable, though it does seem to >> be somewhat theoretical. But in any case, a bug fix should >> have a Fixes tag, which describes the commit that added the bug. >> >> Alternatively, if it is not a bug fix, then it should be targeted at >> net-next (and not have a Fixes tag). Please note that net-next is currently >> closed for the v6.12 merge window. It shold re-open after v6.12-rc1 has >> been released, which I expect to occur about a week for now. You should >> wait for net-next to re-open before posting non-RFC patches for it. >> >> Lastly, when reposting patches, please note the 24h rule. >> https://docs.kernel.org/process/maintainer-netdev.html >> > > Thank you so much for the response and the suggestions. I will follow > everything you have said and whatever I have to. > I was just hurrying to see my patch accepted. Also please prefix your patch the same way that previous changes to this file have been done, that is, the subject should be: net: systemport: Add error pointer checks in bcm_sysport_map_queues() Thank you
Hi Dipendra, kernel test robot noticed the following build errors: [auto build test ERROR on net/main] url: https://github.com/intel-lab-lkp/linux/commits/Dipendra-Khadka/net-Add-error-pointer-check-in-bcmsysport-c/20240923-134407 base: net/main patch link: https://lore.kernel.org/r/20240923053900.1310-1-kdipendra88%40gmail.com patch subject: [PATCH v2 net] net: Add error pointer check in bcmsysport.c config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240924/202409240323.wQPM6V1v-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409240323.wQPM6V1v-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409240323.wQPM6V1v-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/net/ethernet/broadcom/bcmsysport.c:2341:10: error: call to undeclared function 'PRT_ERR'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 2341 | return PRT_ERR(dp); | ^ drivers/net/ethernet/broadcom/bcmsysport.c:2341:10: note: did you mean 'PTR_ERR'? include/linux/err.h:49:33: note: 'PTR_ERR' declared here 49 | static inline long __must_check PTR_ERR(__force const void *ptr) | ^ 1 error generated. vim +/PRT_ERR +2341 drivers/net/ethernet/broadcom/bcmsysport.c 2330 2331 static int bcm_sysport_map_queues(struct net_device *dev, 2332 struct net_device *slave_dev) 2333 { 2334 struct dsa_port *dp = dsa_port_from_netdev(slave_dev); 2335 struct bcm_sysport_priv *priv = netdev_priv(dev); 2336 struct bcm_sysport_tx_ring *ring; 2337 unsigned int num_tx_queues; 2338 unsigned int q, qp, port; 2339 2340 if (IS_ERR(dp)) > 2341 return PRT_ERR(dp); 2342 2343 /* We can't be setting up queue inspection for non directly attached 2344 * switches 2345 */ 2346 if (dp->ds->index) 2347 return 0; 2348 2349 port = dp->index; 2350 2351 /* On SYSTEMPORT Lite we have twice as less queues, so we cannot do a 2352 * 1:1 mapping, we can only do a 2:1 mapping. By reducing the number of 2353 * per-port (slave_dev) network devices queue, we achieve just that. 2354 * This need to happen now before any slave network device is used such 2355 * it accurately reflects the number of real TX queues. 2356 */ 2357 if (priv->is_lite) 2358 netif_set_real_num_tx_queues(slave_dev, 2359 slave_dev->num_tx_queues / 2); 2360 2361 num_tx_queues = slave_dev->real_num_tx_queues; 2362 2363 if (priv->per_port_num_tx_queues && 2364 priv->per_port_num_tx_queues != num_tx_queues) 2365 netdev_warn(slave_dev, "asymmetric number of per-port queues\n"); 2366 2367 priv->per_port_num_tx_queues = num_tx_queues; 2368 2369 for (q = 0, qp = 0; q < dev->num_tx_queues && qp < num_tx_queues; 2370 q++) { 2371 ring = &priv->tx_rings[q]; 2372 2373 if (ring->inspect) 2374 continue; 2375 2376 /* Just remember the mapping actual programming done 2377 * during bcm_sysport_init_tx_ring 2378 */ 2379 ring->switch_queue = qp; 2380 ring->switch_port = port; 2381 ring->inspect = true; 2382 priv->ring_map[qp + port * num_tx_queues] = ring; 2383 qp++; 2384 } 2385 2386 return 0; 2387 } 2388
Hi Dipendra, kernel test robot noticed the following build errors: [auto build test ERROR on net/main] url: https://github.com/intel-lab-lkp/linux/commits/Dipendra-Khadka/net-Add-error-pointer-check-in-bcmsysport-c/20240923-134407 base: net/main patch link: https://lore.kernel.org/r/20240923053900.1310-1-kdipendra88%40gmail.com patch subject: [PATCH v2 net] net: Add error pointer check in bcmsysport.c config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240924/202409240305.PgIkDx1K-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409240305.PgIkDx1K-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409240305.PgIkDx1K-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/net/ethernet/broadcom/bcmsysport.c: In function 'bcm_sysport_map_queues': >> drivers/net/ethernet/broadcom/bcmsysport.c:2341:24: error: implicit declaration of function 'PRT_ERR'; did you mean 'PTR_ERR'? [-Werror=implicit-function-declaration] 2341 | return PRT_ERR(dp); | ^~~~~~~ | PTR_ERR cc1: some warnings being treated as errors vim +2341 drivers/net/ethernet/broadcom/bcmsysport.c 2330 2331 static int bcm_sysport_map_queues(struct net_device *dev, 2332 struct net_device *slave_dev) 2333 { 2334 struct dsa_port *dp = dsa_port_from_netdev(slave_dev); 2335 struct bcm_sysport_priv *priv = netdev_priv(dev); 2336 struct bcm_sysport_tx_ring *ring; 2337 unsigned int num_tx_queues; 2338 unsigned int q, qp, port; 2339 2340 if (IS_ERR(dp)) > 2341 return PRT_ERR(dp); 2342 2343 /* We can't be setting up queue inspection for non directly attached 2344 * switches 2345 */ 2346 if (dp->ds->index) 2347 return 0; 2348 2349 port = dp->index; 2350 2351 /* On SYSTEMPORT Lite we have twice as less queues, so we cannot do a 2352 * 1:1 mapping, we can only do a 2:1 mapping. By reducing the number of 2353 * per-port (slave_dev) network devices queue, we achieve just that. 2354 * This need to happen now before any slave network device is used such 2355 * it accurately reflects the number of real TX queues. 2356 */ 2357 if (priv->is_lite) 2358 netif_set_real_num_tx_queues(slave_dev, 2359 slave_dev->num_tx_queues / 2); 2360 2361 num_tx_queues = slave_dev->real_num_tx_queues; 2362 2363 if (priv->per_port_num_tx_queues && 2364 priv->per_port_num_tx_queues != num_tx_queues) 2365 netdev_warn(slave_dev, "asymmetric number of per-port queues\n"); 2366 2367 priv->per_port_num_tx_queues = num_tx_queues; 2368 2369 for (q = 0, qp = 0; q < dev->num_tx_queues && qp < num_tx_queues; 2370 q++) { 2371 ring = &priv->tx_rings[q]; 2372 2373 if (ring->inspect) 2374 continue; 2375 2376 /* Just remember the mapping actual programming done 2377 * during bcm_sysport_init_tx_ring 2378 */ 2379 ring->switch_queue = qp; 2380 ring->switch_port = port; 2381 ring->inspect = true; 2382 priv->ring_map[qp + port * num_tx_queues] = ring; 2383 qp++; 2384 } 2385 2386 return 0; 2387 } 2388
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index c9faa8540859..97d2ff2329cb 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -2337,6 +2337,9 @@ static int bcm_sysport_map_queues(struct net_device *dev, unsigned int num_tx_queues; unsigned int q, qp, port; + if (IS_ERR(dp)) + return PRT_ERR(dp); + /* We can't be setting up queue inspection for non directly attached * switches */ @@ -2392,6 +2395,9 @@ static int bcm_sysport_unmap_queues(struct net_device *dev, unsigned int num_tx_queues; unsigned int q, qp, port; + if (IS_ERR(dp)) + return PTR_ERR(dp); + port = dp->index; num_tx_queues = slave_dev->real_num_tx_queues;
Add error pointer checks in bcm_sysport_map_queues() and bcm_sysport_unmap_queues() before deferencing 'dp'. Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com> --- v2: - Change the subject of the patch to net v1: https://lore.kernel.org/all/20240922181739.50056-1-kdipendra88@gmail.com/ drivers/net/ethernet/broadcom/bcmsysport.c | 6 ++++++ 1 file changed, 6 insertions(+)