[PATCH v2 net] net: Add error pointer check in bcmsysport.c

[PATCH v2 net] net: Add error pointer check in bcmsysport.c

[Dipendra Khadka]

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(+)

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;
-- 
2.43.0

Re: [PATCH v2 net] net: Add error pointer check in bcmsysport.c

[Simon Horman]

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

-- 
pw-bot: changes-requested

Re: [PATCH v2 net] net: Add error pointer check in bcmsysport.c

[Dipendra Khadka]

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

Re: [PATCH v2 net] net: Add error pointer check in bcmsysport.c

[Florian Fainelli]

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
-- 
Florian

Re: [PATCH v2 net] net: Add error pointer check in bcmsysport.c

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 net] net: Add error pointer check in bcmsysport.c

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki