All of lore.kernel.org
 help / color / mirror / Atom feed
From: Anant Thazhemadam <anant.thazhemadam@gmail.com>
To: Joe Perches <joe@perches.com>
Cc: linux-kernel-mentees@lists.linuxfoundation.org,
	syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com,
	Petko Manolov <petkan@nucleusys.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	linux-usb@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails
Date: Sun, 4 Oct 2020 03:15:29 +0530	[thread overview]
Message-ID: <ed510989-841c-8f2f-73f0-3885eef35a99@gmail.com> (raw)
In-Reply-To: <d44d8c784f9df6f55dcf494c9c21cd11b16338d4.camel@perches.com>


On 04/10/20 3:05 am, Joe Perches wrote:
> On Sun, 2020-10-04 at 02:49 +0530, Anant Thazhemadam wrote:
>> When get_registers() fails, in set_ethernet_addr(),the uninitialized
>> value of node_id gets copied as the address. This can be considered as
>> set_ethernet_addr() itself failing.
> []
>> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> []
>> @@ -909,7 +914,10 @@ static int rtl8150_probe(struct usb_interface *intf,
>>  		goto out1;
>>  	}
>>  	fill_skb_pool(dev);
>> -	set_ethernet_addr(dev);
>> +	if (!set_ethernet_addr(dev)) {
>> +		dev_err(&intf->dev, "assigining a random MAC address\n");
>> +		eth_hw_addr_random(dev->netdev);
> 4 things:
> .
> o Typo for assigning
Oh no. I'm sorry about that.
> o Reverse the assignment and message to show the new random MAC
Ah, okay. That would be more informative.
> o This should use netdev_<level>
Understood.
> o Is this better as error or notification?
>
> 	if (!set_ethernet_addr(dev)) {
> 		eth_hw_addr_random(dev->netdev);
> 		netdev_notice(dev->netdev, "Assigned a random MAC: %pM\n",
> 			      dev->netdev->dev_addr);
> 	}
I thought it might be an error since set_ethernet_addr() did fail after
all.  But making it info seems like a better idea, since technically speaking,
the device is still made accessible.

I'll wait for a day or two, to see if anybody else has any other comments,
and send in a v4 incorporating these changes.

Thanks,
Anant

WARNING: multiple messages have this Message-ID (diff)
From: Anant Thazhemadam <anant.thazhemadam@gmail.com>
To: Joe Perches <joe@perches.com>
Cc: Petko Manolov <petkan@nucleusys.com>,
	syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com,
	netdev@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
	linux-kernel-mentees@lists.linuxfoundation.org,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [Linux-kernel-mentees] [PATCH v3] net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails
Date: Sun, 4 Oct 2020 03:15:29 +0530	[thread overview]
Message-ID: <ed510989-841c-8f2f-73f0-3885eef35a99@gmail.com> (raw)
In-Reply-To: <d44d8c784f9df6f55dcf494c9c21cd11b16338d4.camel@perches.com>


On 04/10/20 3:05 am, Joe Perches wrote:
> On Sun, 2020-10-04 at 02:49 +0530, Anant Thazhemadam wrote:
>> When get_registers() fails, in set_ethernet_addr(),the uninitialized
>> value of node_id gets copied as the address. This can be considered as
>> set_ethernet_addr() itself failing.
> []
>> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> []
>> @@ -909,7 +914,10 @@ static int rtl8150_probe(struct usb_interface *intf,
>>  		goto out1;
>>  	}
>>  	fill_skb_pool(dev);
>> -	set_ethernet_addr(dev);
>> +	if (!set_ethernet_addr(dev)) {
>> +		dev_err(&intf->dev, "assigining a random MAC address\n");
>> +		eth_hw_addr_random(dev->netdev);
> 4 things:
> .
> o Typo for assigning
Oh no. I'm sorry about that.
> o Reverse the assignment and message to show the new random MAC
Ah, okay. That would be more informative.
> o This should use netdev_<level>
Understood.
> o Is this better as error or notification?
>
> 	if (!set_ethernet_addr(dev)) {
> 		eth_hw_addr_random(dev->netdev);
> 		netdev_notice(dev->netdev, "Assigned a random MAC: %pM\n",
> 			      dev->netdev->dev_addr);
> 	}
I thought it might be an error since set_ethernet_addr() did fail after
all.  But making it info seems like a better idea, since technically speaking,
the device is still made accessible.

I'll wait for a day or two, to see if anybody else has any other comments,
and send in a v4 incorporating these changes.

Thanks,
Anant
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

  reply	other threads:[~2020-10-03 21:45 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-03 21:19 [PATCH v3] net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails Anant Thazhemadam
2020-10-03 21:19 ` [Linux-kernel-mentees] " Anant Thazhemadam
2020-10-03 21:35 ` Joe Perches
2020-10-03 21:35   ` [Linux-kernel-mentees] " Joe Perches
2020-10-03 21:45   ` Anant Thazhemadam [this message]
2020-10-03 21:45     ` Anant Thazhemadam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ed510989-841c-8f2f-73f0-3885eef35a99@gmail.com \
    --to=anant.thazhemadam@gmail.com \
    --cc=davem@davemloft.net \
    --cc=joe@perches.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=petkan@nucleusys.com \
    --cc=syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.