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 <[email protected]>
drivers/net/8139cp.c | 24 +++++++++++++++++++++++-
1 files changed, 23 insertions(+), 1 deletions(-)
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,
On Thu, 12 Mar 2009 17:27:31 +0100
Jiri Pirko <[email protected]> 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
Thu, Mar 12, 2009 at 06:11:21PM CET, [email protected] wrote:
>On Thu, 12 Mar 2009 17:27:31 +0100
>Jiri Pirko <[email protected]> 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
Jiri Pirko wrote:
> Thu, Mar 12, 2009 at 06:11:21PM CET, [email protected] wrote:
>> On Thu, 12 Mar 2009 17:27:31 +0100
>> Jiri Pirko <[email protected]> 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
Fri, Mar 13, 2009 at 10:01:21AM CET, [email protected] wrote:
>Jiri Pirko wrote:
>> Thu, Mar 12, 2009 at 06:11:21PM CET, [email protected] wrote:
>>> On Thu, 12 Mar 2009 17:27:31 +0100
>>> Jiri Pirko <[email protected]> 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
Jiri Pirko wrote:
> Fri, Mar 13, 2009 at 10:01:21AM CET, [email protected] wrote:
>> Jiri Pirko wrote:
>>> Thu, Mar 12, 2009 at 06:11:21PM CET, [email protected] wrote:
>>>> On Thu, 12 Mar 2009 17:27:31 +0100
>>>> Jiri Pirko <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Ivan Vecera wrote:
> Jiri Pirko wrote:
>> Fri, Mar 13, 2009 at 10:01:21AM CET, [email protected] wrote:
>>> Jiri Pirko wrote:
>>>> Thu, Mar 12, 2009 at 06:11:21PM CET, [email protected] wrote:
>>>>> On Thu, 12 Mar 2009 17:27:31 +0100
>>>>> Jiri Pirko <[email protected]> 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
Jeff Garzik wrote:
> Ivan Vecera wrote:
>> Jiri Pirko wrote:
>>> Fri, Mar 13, 2009 at 10:01:21AM CET, [email protected] wrote:
>>>> Jiri Pirko wrote:
>>>>> Thu, Mar 12, 2009 at 06:11:21PM CET, [email protected] wrote:
>>>>>> On Thu, 12 Mar 2009 17:27:31 +0100
>>>>>> Jiri Pirko <[email protected]> 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
>
>
>
From: Jiri Pirko <[email protected]>
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 <[email protected]>
Applied to net-next-2.6