| Message ID | 20090312162730.GA20153@psychotron.englab.brq.redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
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
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,
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