From: Robin Murphy <robin.murphy@arm.com> To: Jeremy Linton <jeremy.linton@arm.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Cc: "steve.glendinning@shawell.net" <steve.glendinning@shawell.net>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "rafael.j.wysocki@intel.com" <rafael.j.wysocki@intel.com>, "suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>, Catalin Marinas <Catalin.Marinas@arm.com>, "grant.likely@linaro.org" <grant.likely@linaro.org> Subject: Re: [PATCH 1/2] Add a matching set of device_ functions for determining mac/phy Date: Thu, 13 Aug 2015 12:57:07 +0100 [thread overview] Message-ID: <55CC8613.1040709@arm.com> (raw) In-Reply-To: <1439417187-21411-2-git-send-email-jeremy.linton@arm.com> Hi Jeremy, On 12/08/15 23:06, Jeremy Linton wrote: [...] > +static void *device_get_mac_addr(struct device *dev, > + const char *name, char *addr, > + int alen) > +{ > + int ret = device_property_read_u8_array(dev, name, addr, alen); > + > + if (ret == 0 && is_valid_ether_addr(addr)) > + return addr; > + return NULL; > +} Not sure I understand the logic here - "return the same thing we were given if we updated it, or null if we didn't". It's only indicating success/failure (the caller can perfectly well cast its own buffer to a void * if it needs to), so why wouldn't you just return a normal int error code? > +/** > + * Search the device tree for the best MAC address to use. 'mac-address' is > + * checked first, because that is supposed to contain to "most recent" MAC > + * address. If that isn't set, then 'local-mac-address' is checked next, > + * because that is the default address. If that isn't set, then the obsolete > + * 'address' is checked, just in case we're using an old device tree. > + * > + * Note that the 'address' property is supposed to contain a virtual address of > + * the register set, but some DTS files have redefined that property to be the > + * MAC address. > + * > + * All-zero MAC addresses are rejected, because those could be properties that > + * exist in the device tree, but were not set by U-Boot. For example, the > + * DTS could define 'mac-address' and 'local-mac-address', with zero MAC > + * addresses. Some older U-Boots only initialized 'local-mac-address'. In > + * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists > + * but is all zeros. > +*/ > +void *device_get_mac_address(struct device *dev, char *addr, int alen) > +{ > + addr = device_get_mac_addr(dev, "mac-address", addr, alen); > + if (addr) > + return addr; > + > + addr = device_get_mac_addr(dev, "local-mac-address", addr, alen); > + if (addr) > + return addr; > + > + return device_get_mac_addr(dev, "address", addr, alen); > +} > +EXPORT_SYMBOL(device_get_mac_address); Same here, it's not at all apparent why this should return a void * instead of an int (or even possibly bool). of_get_mac_address is giving its caller back a _new_ pointer they didn't know about before; this isn't. Robin.
WARNING: multiple messages have this Message-ID (diff)
From: robin.murphy@arm.com (Robin Murphy) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/2] Add a matching set of device_ functions for determining mac/phy Date: Thu, 13 Aug 2015 12:57:07 +0100 [thread overview] Message-ID: <55CC8613.1040709@arm.com> (raw) In-Reply-To: <1439417187-21411-2-git-send-email-jeremy.linton@arm.com> Hi Jeremy, On 12/08/15 23:06, Jeremy Linton wrote: [...] > +static void *device_get_mac_addr(struct device *dev, > + const char *name, char *addr, > + int alen) > +{ > + int ret = device_property_read_u8_array(dev, name, addr, alen); > + > + if (ret == 0 && is_valid_ether_addr(addr)) > + return addr; > + return NULL; > +} Not sure I understand the logic here - "return the same thing we were given if we updated it, or null if we didn't". It's only indicating success/failure (the caller can perfectly well cast its own buffer to a void * if it needs to), so why wouldn't you just return a normal int error code? > +/** > + * Search the device tree for the best MAC address to use. 'mac-address' is > + * checked first, because that is supposed to contain to "most recent" MAC > + * address. If that isn't set, then 'local-mac-address' is checked next, > + * because that is the default address. If that isn't set, then the obsolete > + * 'address' is checked, just in case we're using an old device tree. > + * > + * Note that the 'address' property is supposed to contain a virtual address of > + * the register set, but some DTS files have redefined that property to be the > + * MAC address. > + * > + * All-zero MAC addresses are rejected, because those could be properties that > + * exist in the device tree, but were not set by U-Boot. For example, the > + * DTS could define 'mac-address' and 'local-mac-address', with zero MAC > + * addresses. Some older U-Boots only initialized 'local-mac-address'. In > + * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists > + * but is all zeros. > +*/ > +void *device_get_mac_address(struct device *dev, char *addr, int alen) > +{ > + addr = device_get_mac_addr(dev, "mac-address", addr, alen); > + if (addr) > + return addr; > + > + addr = device_get_mac_addr(dev, "local-mac-address", addr, alen); > + if (addr) > + return addr; > + > + return device_get_mac_addr(dev, "address", addr, alen); > +} > +EXPORT_SYMBOL(device_get_mac_address); Same here, it's not at all apparent why this should return a void * instead of an int (or even possibly bool). of_get_mac_address is giving its caller back a _new_ pointer they didn't know about before; this isn't. Robin.
next prev parent reply other threads:[~2015-08-13 11:57 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-08-12 22:06 [PATCH 0/2] Enable smsc911x for use with ACPI Jeremy Linton 2015-08-12 22:06 ` Jeremy Linton 2015-08-12 22:06 ` [PATCH 1/2] Add a matching set of device_ functions for determining mac/phy Jeremy Linton 2015-08-12 22:06 ` Jeremy Linton 2015-08-12 22:13 ` Florian Fainelli 2015-08-12 22:13 ` Florian Fainelli 2015-08-14 15:55 ` Jeremy Linton 2015-08-14 15:55 ` Jeremy Linton 2015-08-13 11:57 ` Robin Murphy [this message] 2015-08-13 11:57 ` Robin Murphy 2015-08-13 14:24 ` Jeremy Linton 2015-08-13 14:24 ` Jeremy Linton 2015-08-12 22:06 ` [PATCH 2/2] Convert smsc911x to use ACPI as well as DT Jeremy Linton 2015-08-12 22:06 ` Jeremy Linton 2015-08-13 8:27 ` Graeme Gregory 2015-08-13 8:27 ` Graeme Gregory 2015-08-13 9:01 ` Lorenzo Pieralisi 2015-08-13 9:01 ` Lorenzo Pieralisi 2015-08-13 9:38 ` Graeme Gregory 2015-08-13 9:38 ` Graeme Gregory 2015-08-13 10:30 ` Lorenzo Pieralisi 2015-08-13 10:30 ` Lorenzo Pieralisi 2015-09-09 16:10 ` Marc Zyngier 2015-09-09 16:10 ` Marc Zyngier 2015-09-23 17:22 ` Jeremy Linton 2015-09-23 17:22 ` Jeremy Linton 2015-09-23 17:46 ` Marc Zyngier 2015-09-23 17:46 ` Marc Zyngier 2015-09-23 17:57 ` Sudeep Holla 2015-09-23 17:57 ` Sudeep Holla 2015-09-24 9:20 ` Catalin Marinas 2015-09-24 9:20 ` Catalin Marinas 2015-11-02 15:48 ` Jeremy Linton 2015-11-02 15:48 ` Jeremy Linton 2015-09-23 18:41 ` David Woodhouse 2015-09-23 18:41 ` David Woodhouse 2015-09-23 20:51 ` Rafael J. Wysocki 2015-09-23 20:51 ` Rafael J. Wysocki 2015-09-23 21:03 ` David Woodhouse 2015-09-23 21:03 ` David Woodhouse 2015-09-23 23:56 ` Hanjun Guo 2015-09-23 23:56 ` Hanjun Guo 2015-09-24 8:16 ` David Woodhouse 2015-09-24 8:16 ` David Woodhouse 2015-09-24 10:31 ` Catalin Marinas 2015-09-24 10:31 ` Catalin Marinas 2015-09-24 11:52 ` David Woodhouse 2015-09-24 11:52 ` David Woodhouse 2015-09-24 14:01 ` Lorenzo Pieralisi 2015-09-24 14:01 ` Lorenzo Pieralisi 2015-09-24 14:31 ` David Woodhouse 2015-09-24 14:31 ` David Woodhouse 2015-09-24 15:15 ` Catalin Marinas 2015-09-24 15:15 ` Catalin Marinas 2015-09-24 18:10 ` David Woodhouse 2015-09-24 18:10 ` David Woodhouse 2015-09-25 15:28 ` Catalin Marinas 2015-09-25 15:28 ` Catalin Marinas 2015-09-26 2:16 ` Rafael J. Wysocki 2015-09-26 2:16 ` Rafael J. Wysocki 2015-09-26 15:20 ` David Woodhouse 2015-09-26 15:20 ` David Woodhouse 2015-10-01 2:23 ` Al Stone 2015-10-01 2:23 ` Al Stone 2015-10-06 0:20 ` Charles Garcia-Tobin 2015-10-06 0:20 ` Charles Garcia-Tobin 2015-10-06 11:08 ` David Woodhouse 2015-10-06 11:08 ` David Woodhouse 2015-10-08 0:11 ` Rafael J. Wysocki 2015-10-08 0:11 ` Rafael J. Wysocki 2015-08-14 0:00 ` [PATCH 0/2] Enable smsc911x for use with ACPI David Miller 2015-08-14 0:00 ` David Miller
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=55CC8613.1040709@arm.com \ --to=robin.murphy@arm.com \ --cc=Catalin.Marinas@arm.com \ --cc=grant.likely@linaro.org \ --cc=jeremy.linton@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=netdev@vger.kernel.org \ --cc=rafael.j.wysocki@intel.com \ --cc=steve.glendinning@shawell.net \ --cc=suravee.suthikulpanit@amd.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: linkBe 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.