Patchwork 8139cp: allow to set mac address on running device

login
register
mail settings
Submitter Jiri Pirko
Date March 12, 2009, 4:27 p.m.
Message ID <20090312162730.GA20153@psychotron.englab.brq.redhat.com>
Download mbox | patch
Permalink /patch/24350/
State Accepted
Delegated to: David Miller
Headers show

Comments

Jiri Pirko - March 12, 2009, 4:27 p.m.
So far there was not a chance to set a mac address on running 8139cp device.
This is for example needed when you want to use this NIC as a bonding slave in
bonding device in mode balance-alb. This simple patch allows it.

Jirka


Signed-off-by: Jiri Pirko <jpirko@redhat.com>

 drivers/net/8139cp.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Schmidt - March 12, 2009, 5:11 p.m.
On Thu, 12 Mar 2009 17:27:31 +0100
Jiri Pirko <jpirko@redhat.com> wrote:

> +	cpw32_f(MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr +
> 0)));
> +	cpw32_f(MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr +
> 4)));

You're writing to the card, so using *_to_cpu looks suspicious.

Michal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko - March 12, 2009, 5:46 p.m.
Thu, Mar 12, 2009 at 06:11:21PM CET, mschmidt@redhat.com wrote:
>On Thu, 12 Mar 2009 17:27:31 +0100
>Jiri Pirko <jpirko@redhat.com> wrote:
>
>> +	cpw32_f(MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>> 0)));
>> +	cpw32_f(MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>> 4)));
>
>You're writing to the card, so using *_to_cpu looks suspicious.
Well, I'm using the same approach as it is already done in function
cp_init_hw(). Quote:

 /* Restore our idea of the MAC address. */
 cpw32_f (MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr + 0)));
 cpw32_f (MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr + 4)));

Jirka
>
>Michal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Vecera - March 13, 2009, 9:01 a.m.
Jiri Pirko wrote:
> Thu, Mar 12, 2009 at 06:11:21PM CET, mschmidt@redhat.com wrote:
>> On Thu, 12 Mar 2009 17:27:31 +0100
>> Jiri Pirko <jpirko@redhat.com> wrote:
>>
>>> +	cpw32_f(MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>> 0)));
>>> +	cpw32_f(MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>> 4)));
>> You're writing to the card, so using *_to_cpu looks suspicious.
> Well, I'm using the same approach as it is already done in function
> cp_init_hw(). Quote:
> 
>  /* Restore our idea of the MAC address. */
>  cpw32_f (MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr + 0)));
>  cpw32_f (MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr + 4)));
> 
Yes, that's right but I would use more cleaner approach:
===
u32 low, high;
low  = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
high = addr[4] | (addr[5] << 8);
cpw32_f(MAC0 + 0, low);
cpw32_f(MAC0 + 4, high);
===

Ivan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko - March 13, 2009, 11:16 a.m.
Fri, Mar 13, 2009 at 10:01:21AM CET, ivecera@redhat.com wrote:
>Jiri Pirko wrote:
>> Thu, Mar 12, 2009 at 06:11:21PM CET, mschmidt@redhat.com wrote:
>>> On Thu, 12 Mar 2009 17:27:31 +0100
>>> Jiri Pirko <jpirko@redhat.com> wrote:
>>>
>>>> +	cpw32_f(MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>>> 0)));
>>>> +	cpw32_f(MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>>> 4)));
>>> You're writing to the card, so using *_to_cpu looks suspicious.
>> Well, I'm using the same approach as it is already done in function
>> cp_init_hw(). Quote:
>> 
>>  /* Restore our idea of the MAC address. */
>>  cpw32_f (MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr + 0)));
>>  cpw32_f (MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr + 4)));
>> 
>Yes, that's right but I would use more cleaner approach:
>===
>u32 low, high;
>low  = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
>high = addr[4] | (addr[5] << 8);
>cpw32_f(MAC0 + 0, low);
>cpw32_f(MAC0 + 4, high);
>===
Well, I have no problem with this (in fact I like this more). I just wanted to
stay consistent to existing code. Maybe it would be good to change this chunk
of code in cp_init_hw() too, don't you think?

Jirka
>
>Ivan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Vecera - March 13, 2009, 1:11 p.m.
Jiri Pirko wrote:
> Fri, Mar 13, 2009 at 10:01:21AM CET, ivecera@redhat.com wrote:
>> Jiri Pirko wrote:
>>> Thu, Mar 12, 2009 at 06:11:21PM CET, mschmidt@redhat.com wrote:
>>>> On Thu, 12 Mar 2009 17:27:31 +0100
>>>> Jiri Pirko <jpirko@redhat.com> wrote:
>>>>
>>>>> +	cpw32_f(MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>>>> 0)));
>>>>> +	cpw32_f(MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>>>> 4)));
>>>> You're writing to the card, so using *_to_cpu looks suspicious.
>>> Well, I'm using the same approach as it is already done in function
>>> cp_init_hw(). Quote:
>>>
>>>  /* Restore our idea of the MAC address. */
>>>  cpw32_f (MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr + 0)));
>>>  cpw32_f (MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr + 4)));
>>>
>> Yes, that's right but I would use more cleaner approach:
>> ===
>> u32 low, high;
>> low  = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
>> high = addr[4] | (addr[5] << 8);
>> cpw32_f(MAC0 + 0, low);
>> cpw32_f(MAC0 + 4, high);
>> ===
> Well, I have no problem with this (in fact I like this more). I just wanted to
> stay consistent to existing code. Maybe it would be good to change this chunk
> of code in cp_init_hw() too, don't you think?
Yes, you're right.
> 
> Jirka
>> Ivan
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - March 13, 2009, 1:36 p.m.
Ivan Vecera wrote:
> Jiri Pirko wrote:
>> Fri, Mar 13, 2009 at 10:01:21AM CET, ivecera@redhat.com wrote:
>>> Jiri Pirko wrote:
>>>> Thu, Mar 12, 2009 at 06:11:21PM CET, mschmidt@redhat.com wrote:
>>>>> On Thu, 12 Mar 2009 17:27:31 +0100
>>>>> Jiri Pirko <jpirko@redhat.com> wrote:
>>>>>
>>>>>> +	cpw32_f(MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>>>>> 0)));
>>>>>> +	cpw32_f(MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>>>>> 4)));
>>>>> You're writing to the card, so using *_to_cpu looks suspicious.
>>>> Well, I'm using the same approach as it is already done in function
>>>> cp_init_hw(). Quote:
>>>>
>>>>  /* Restore our idea of the MAC address. */
>>>>  cpw32_f (MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr + 0)));
>>>>  cpw32_f (MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr + 4)));
>>>>
>>> Yes, that's right but I would use more cleaner approach:
>>> ===
>>> u32 low, high;
>>> low  = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
>>> high = addr[4] | (addr[5] << 8);
>>> cpw32_f(MAC0 + 0, low);
>>> cpw32_f(MAC0 + 4, high);
>>> ===
>> Well, I have no problem with this (in fact I like this more). I just wanted to
>> stay consistent to existing code. Maybe it would be good to change this chunk
>> of code in cp_init_hw() too, don't you think?
> Yes, you're right.

The existing code is correct, and works.  How about just leaving it alone?

You can grep around and see other drivers doing this when necessary, too.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Vecera - March 13, 2009, 1:39 p.m.
Jeff Garzik wrote:
> Ivan Vecera wrote:
>> Jiri Pirko wrote:
>>> Fri, Mar 13, 2009 at 10:01:21AM CET, ivecera@redhat.com wrote:
>>>> Jiri Pirko wrote:
>>>>> Thu, Mar 12, 2009 at 06:11:21PM CET, mschmidt@redhat.com wrote:
>>>>>> On Thu, 12 Mar 2009 17:27:31 +0100
>>>>>> Jiri Pirko <jpirko@redhat.com> wrote:
>>>>>>
>>>>>>> +	cpw32_f(MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>>>>>> 0)));
>>>>>>> +	cpw32_f(MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr +
>>>>>>> 4)));
>>>>>> You're writing to the card, so using *_to_cpu looks suspicious.
>>>>> Well, I'm using the same approach as it is already done in function
>>>>> cp_init_hw(). Quote:
>>>>>
>>>>>  /* Restore our idea of the MAC address. */
>>>>>  cpw32_f (MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr + 0)));
>>>>>  cpw32_f (MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr + 4)));
>>>>>
>>>> Yes, that's right but I would use more cleaner approach:
>>>> ===
>>>> u32 low, high;
>>>> low  = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
>>>> high = addr[4] | (addr[5] << 8);
>>>> cpw32_f(MAC0 + 0, low);
>>>> cpw32_f(MAC0 + 4, high);
>>>> ===
>>> Well, I have no problem with this (in fact I like this more). I just wanted to
>>> stay consistent to existing code. Maybe it would be good to change this chunk
>>> of code in cp_init_hw() too, don't you think?
>> Yes, you're right.
> 
> The existing code is correct, and works.  How about just leaving it alone?
+1

Ivan
> 
> You can grep around and see other drivers doing this when necessary, too.
> 
> 	Jeff
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - March 13, 2009, 6:47 p.m.
From: Jiri Pirko <jpirko@redhat.com>
Date: Thu, 12 Mar 2009 17:27:31 +0100

> So far there was not a chance to set a mac address on running 8139cp device.
> This is for example needed when you want to use this NIC as a bonding slave in
> bonding device in mode balance-alb. This simple patch allows it.
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Applied to net-next-2.6
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/8139cp.c b/drivers/net/8139cp.c
index 4e19ae3..13b708a 100644
--- a/drivers/net/8139cp.c
+++ b/drivers/net/8139cp.c
@@ -1602,6 +1602,28 @@  static int cp_ioctl (struct net_device *dev, struct ifreq *rq, int cmd)
 	return rc;
 }
 
+static int cp_set_mac_address(struct net_device *dev, void *p)
+{
+	struct cp_private *cp = netdev_priv(dev);
+	struct sockaddr *addr = p;
+
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
+
+	spin_lock_irq(&cp->lock);
+
+	cpw8_f(Cfg9346, Cfg9346_Unlock);
+	cpw32_f(MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr + 0)));
+	cpw32_f(MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr + 4)));
+	cpw8_f(Cfg9346, Cfg9346_Lock);
+
+	spin_unlock_irq(&cp->lock);
+
+	return 0;
+}
+
 /* Serial EEPROM section. */
 
 /*  EEPROM_Ctrl bits. */
@@ -1821,7 +1843,7 @@  static const struct net_device_ops cp_netdev_ops = {
 	.ndo_open		= cp_open,
 	.ndo_stop		= cp_close,
 	.ndo_validate_addr	= eth_validate_addr,
-	.ndo_set_mac_address 	= eth_mac_addr,
+	.ndo_set_mac_address 	= cp_set_mac_address,
 	.ndo_set_multicast_list	= cp_set_rx_mode,
 	.ndo_get_stats		= cp_get_stats,
 	.ndo_do_ioctl		= cp_ioctl,