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

[Florian Fainelli <f.fainelli@gmail.com>]

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