| Message ID | 20191218164239.29768-1-michael@walle.cc |
|---|---|
| State | Superseded |
| Delegated to: | Priyanka Jain |
| Headers | show |
| Series | dm: uclass: don't assign aliased seq numbers | expand |
Hi Michael, On 12/18/2019 5:42 PM, Michael Walle wrote: > If there are aliases for an uclass, set the base for the "dynamically" > allocated numbers next to the highest alias. > > Please note, that this might lead to holes in the sequences, depending > on the device tree. For example if there is only an alias "ethernet1", > the next device seq number would be 2. > > In particular this fixes a problem with boards which are using ethernet > aliases but also might have network add-in cards like the E1000. If the > board is started with the add-in card and depending on the order of the > drivers, the E1000 might occupy the first ethernet device and mess up > all the hardware addresses, because the devices are now shifted by one. > > As a side effect, this should also make the following commits > superfluous: > - 7f3289bf6d ("dm: device: Request next sequence number") > - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") > Although I don't understand the root cause of the said problem. > > Thomas, Michal, could you please test this and then I'd add a second > patch removing the old code. > > Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> > Cc: Michal Simek <michal.simek@xilinx.com> > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/core/uclass.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c > index c520ef113a..c3b325141a 100644 > --- a/drivers/core/uclass.c > +++ b/drivers/core/uclass.c > @@ -675,13 +675,14 @@ int uclass_unbind_device(struct udevice *dev) > > int uclass_resolve_seq(struct udevice *dev) > { > + struct uclass *uc = dev->uclass; > + struct uclass_driver *uc_drv = uc->uc_drv; > struct udevice *dup; > - int seq; > + int seq = 0; > int ret; > > assert(dev->seq == -1); > - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, dev->req_seq, > - false, &dup); > + ret = uclass_find_device_by_seq(uc_drv->id, dev->req_seq, false, &dup); > if (!ret) { > dm_warn("Device '%s': seq %d is in use by '%s'\n", > dev->name, dev->req_seq, dup->name); > @@ -693,9 +694,16 @@ int uclass_resolve_seq(struct udevice *dev) > return ret; > } > > - for (seq = 0; seq < DM_MAX_SEQ; seq++) { > - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, seq, > - false, &dup); > + if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) && > + (uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) { > + /* dev_read_alias_highest_id() will return -1 if there no nits: there /is/ no and multi-line comment starts with just /* on the 1st line > + * alias. Thus we can always add one. > + */ > + seq = dev_read_alias_highest_id(uc_drv->name) + 1; > + } > + > + for (; seq < DM_MAX_SEQ; seq++) { > + ret = uclass_find_device_by_seq(uc_drv->id, seq, false, &dup); > if (ret == -ENODEV) > break; > if (ret) > Reviewed-by: Alex Marginean <alexandru.marginean@nxp.com> Tested-by: Alex Marginean <alexandru.marginean@nxp.com> This looks nice. One thing I noticed is that 'dm tree' indexes now don't match dev->seq which can be a bit confusing. The index is produced by dev_get_uclass_index, which in effect counts devices in the uclass. Any issue if 'dm tree' prints dev->seq instead and maybe call the column Seq rather than Index? Thanks! Alex
Am 2019-12-18 21:00, schrieb Alexandru Marginean: > Hi Michael, > > On 12/18/2019 5:42 PM, Michael Walle wrote: >> If there are aliases for an uclass, set the base for the "dynamically" >> allocated numbers next to the highest alias. >> >> Please note, that this might lead to holes in the sequences, depending >> on the device tree. For example if there is only an alias "ethernet1", >> the next device seq number would be 2. >> >> In particular this fixes a problem with boards which are using >> ethernet >> aliases but also might have network add-in cards like the E1000. If >> the >> board is started with the add-in card and depending on the order of >> the >> drivers, the E1000 might occupy the first ethernet device and mess up >> all the hardware addresses, because the devices are now shifted by >> one. >> >> As a side effect, this should also make the following commits >> superfluous: >> - 7f3289bf6d ("dm: device: Request next sequence number") >> - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") >> Although I don't understand the root cause of the said problem. >> >> Thomas, Michal, could you please test this and then I'd add a second >> patch removing the old code. >> >> Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> >> Cc: Michal Simek <michal.simek@xilinx.com> >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/core/uclass.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c >> index c520ef113a..c3b325141a 100644 >> --- a/drivers/core/uclass.c >> +++ b/drivers/core/uclass.c >> @@ -675,13 +675,14 @@ int uclass_unbind_device(struct udevice *dev) >> int uclass_resolve_seq(struct udevice *dev) >> { >> + struct uclass *uc = dev->uclass; >> + struct uclass_driver *uc_drv = uc->uc_drv; >> struct udevice *dup; >> - int seq; >> + int seq = 0; >> int ret; >> assert(dev->seq == -1); >> - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, >> dev->req_seq, >> - false, &dup); >> + ret = uclass_find_device_by_seq(uc_drv->id, dev->req_seq, false, >> &dup); >> if (!ret) { >> dm_warn("Device '%s': seq %d is in use by '%s'\n", >> dev->name, dev->req_seq, dup->name); >> @@ -693,9 +694,16 @@ int uclass_resolve_seq(struct udevice *dev) >> return ret; >> } >> - for (seq = 0; seq < DM_MAX_SEQ; seq++) { >> - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, seq, >> - false, &dup); >> + if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) && >> + (uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) { >> + /* dev_read_alias_highest_id() will return -1 if there no > > nits: there /is/ no and multi-line comment starts with just /* on the > 1st line yeah, I'd prefer that style too. But checkpatch.pl would complain about it.. or so I thought.. well seems only to be the case in linux and only in "NETWORKING_BLOCK_COMMENT_STYLE" (only for net/ and drivers/net/). I'll definitely fix that ;) > >> + * alias. Thus we can always add one. >> + */ >> + seq = dev_read_alias_highest_id(uc_drv->name) + 1; >> + } >> + >> + for (; seq < DM_MAX_SEQ; seq++) { >> + ret = uclass_find_device_by_seq(uc_drv->id, seq, false, &dup); >> if (ret == -ENODEV) >> break; >> if (ret) >> > > > Reviewed-by: Alex Marginean <alexandru.marginean@nxp.com> > Tested-by: Alex Marginean <alexandru.marginean@nxp.com> > > This looks nice. > One thing I noticed is that 'dm tree' indexes now don't match dev->seq > which can be a bit confusing. The index is produced by > dev_get_uclass_index, which in effect counts devices in the uclass. > Any issue if 'dm tree' prints dev->seq instead and maybe call the > column Seq rather than Index? Mhh, did this ever match if req_seq/aliases was used? "dm uclass" dumps the seq and req_seq. So I don't know if that was ever intended to match. But I'm open to suggestions. -michael
On 12/18/2019 10:45 PM, Michael Walle wrote: > Am 2019-12-18 21:00, schrieb Alexandru Marginean: >> Hi Michael, >> >> On 12/18/2019 5:42 PM, Michael Walle wrote: >>> If there are aliases for an uclass, set the base for the "dynamically" >>> allocated numbers next to the highest alias. >>> >>> Please note, that this might lead to holes in the sequences, depending >>> on the device tree. For example if there is only an alias "ethernet1", >>> the next device seq number would be 2. >>> >>> In particular this fixes a problem with boards which are using ethernet >>> aliases but also might have network add-in cards like the E1000. If the >>> board is started with the add-in card and depending on the order of the >>> drivers, the E1000 might occupy the first ethernet device and mess up >>> all the hardware addresses, because the devices are now shifted by one. >>> >>> As a side effect, this should also make the following commits >>> superfluous: >>> - 7f3289bf6d ("dm: device: Request next sequence number") >>> - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") >>> Although I don't understand the root cause of the said problem. >>> >>> Thomas, Michal, could you please test this and then I'd add a second >>> patch removing the old code. >>> >>> Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> >>> Cc: Michal Simek <michal.simek@xilinx.com> >>> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> drivers/core/uclass.c | 20 ++++++++++++++------ >>> 1 file changed, 14 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c >>> index c520ef113a..c3b325141a 100644 >>> --- a/drivers/core/uclass.c >>> +++ b/drivers/core/uclass.c >>> @@ -675,13 +675,14 @@ int uclass_unbind_device(struct udevice *dev) >>> int uclass_resolve_seq(struct udevice *dev) >>> { >>> + struct uclass *uc = dev->uclass; >>> + struct uclass_driver *uc_drv = uc->uc_drv; >>> struct udevice *dup; >>> - int seq; >>> + int seq = 0; >>> int ret; >>> assert(dev->seq == -1); >>> - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, >>> dev->req_seq, >>> - false, &dup); >>> + ret = uclass_find_device_by_seq(uc_drv->id, dev->req_seq, false, >>> &dup); >>> if (!ret) { >>> dm_warn("Device '%s': seq %d is in use by '%s'\n", >>> dev->name, dev->req_seq, dup->name); >>> @@ -693,9 +694,16 @@ int uclass_resolve_seq(struct udevice *dev) >>> return ret; >>> } >>> - for (seq = 0; seq < DM_MAX_SEQ; seq++) { >>> - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, seq, >>> - false, &dup); >>> + if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) && >>> + (uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) { >>> + /* dev_read_alias_highest_id() will return -1 if there no >> >> nits: there /is/ no and multi-line comment starts with just /* on the >> 1st line > > yeah, I'd prefer that style too. But checkpatch.pl would complain > about it.. or so I thought.. well seems only to be the case in linux and > only in "NETWORKING_BLOCK_COMMENT_STYLE" (only for net/ and drivers/net/). > > I'll definitely fix that ;) > >> >>> + * alias. Thus we can always add one. >>> + */ >>> + seq = dev_read_alias_highest_id(uc_drv->name) + 1; >>> + } >>> + >>> + for (; seq < DM_MAX_SEQ; seq++) { >>> + ret = uclass_find_device_by_seq(uc_drv->id, seq, false, &dup); >>> if (ret == -ENODEV) >>> break; >>> if (ret) >>> >> >> >> Reviewed-by: Alex Marginean <alexandru.marginean@nxp.com> >> Tested-by: Alex Marginean <alexandru.marginean@nxp.com> >> >> This looks nice. >> One thing I noticed is that 'dm tree' indexes now don't match dev->seq >> which can be a bit confusing. The index is produced by >> dev_get_uclass_index, which in effect counts devices in the uclass. >> Any issue if 'dm tree' prints dev->seq instead and maybe call the >> column Seq rather than Index? > > Mhh, did this ever match if req_seq/aliases was used? "dm uclass" dumps > the seq and req_seq. So I don't know if that was ever intended to match. > But I'm open to suggestions. > > -michael It's certainly not something that should be part of this patch and since I think it should be changed I can send a patch for it. Thanks! Alex
Hi Michael, On Wed, 18 Dec 2019 at 18:42, Michael Walle <michael@walle.cc> wrote: > > If there are aliases for an uclass, set the base for the "dynamically" > allocated numbers next to the highest alias. > > Please note, that this might lead to holes in the sequences, depending > on the device tree. For example if there is only an alias "ethernet1", > the next device seq number would be 2. > > In particular this fixes a problem with boards which are using ethernet > aliases but also might have network add-in cards like the E1000. If the > board is started with the add-in card and depending on the order of the > drivers, the E1000 might occupy the first ethernet device and mess up > all the hardware addresses, because the devices are now shifted by one. > > As a side effect, this should also make the following commits > superfluous: > - 7f3289bf6d ("dm: device: Request next sequence number") > - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") > Although I don't understand the root cause of the said problem. > > Thomas, Michal, could you please test this and then I'd add a second > patch removing the old code. > > Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> > Cc: Michal Simek <michal.simek@xilinx.com> > > Signed-off-by: Michael Walle <michael@walle.cc> > --- This sounds so obvious that it makes me wonder why it hasn't been the behavior thus far. Acked-by: Vladimir Oltean <olteanv@gmail.com> > drivers/core/uclass.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c > index c520ef113a..c3b325141a 100644 > --- a/drivers/core/uclass.c > +++ b/drivers/core/uclass.c > @@ -675,13 +675,14 @@ int uclass_unbind_device(struct udevice *dev) > > int uclass_resolve_seq(struct udevice *dev) > { > + struct uclass *uc = dev->uclass; > + struct uclass_driver *uc_drv = uc->uc_drv; > struct udevice *dup; > - int seq; > + int seq = 0; > int ret; > > assert(dev->seq == -1); > - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, dev->req_seq, > - false, &dup); > + ret = uclass_find_device_by_seq(uc_drv->id, dev->req_seq, false, &dup); > if (!ret) { > dm_warn("Device '%s': seq %d is in use by '%s'\n", > dev->name, dev->req_seq, dup->name); > @@ -693,9 +694,16 @@ int uclass_resolve_seq(struct udevice *dev) > return ret; > } > > - for (seq = 0; seq < DM_MAX_SEQ; seq++) { > - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, seq, > - false, &dup); > + if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) && > + (uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) { > + /* dev_read_alias_highest_id() will return -1 if there no > + * alias. Thus we can always add one. > + */ > + seq = dev_read_alias_highest_id(uc_drv->name) + 1; > + } > + > + for (; seq < DM_MAX_SEQ; seq++) { > + ret = uclass_find_device_by_seq(uc_drv->id, seq, false, &dup); > if (ret == -ENODEV) > break; > if (ret) > -- > 2.20.1 >
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index c520ef113a..c3b325141a 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -675,13 +675,14 @@ int uclass_unbind_device(struct udevice *dev) int uclass_resolve_seq(struct udevice *dev) { + struct uclass *uc = dev->uclass; + struct uclass_driver *uc_drv = uc->uc_drv; struct udevice *dup; - int seq; + int seq = 0; int ret; assert(dev->seq == -1); - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, dev->req_seq, - false, &dup); + ret = uclass_find_device_by_seq(uc_drv->id, dev->req_seq, false, &dup); if (!ret) { dm_warn("Device '%s': seq %d is in use by '%s'\n", dev->name, dev->req_seq, dup->name); @@ -693,9 +694,16 @@ int uclass_resolve_seq(struct udevice *dev) return ret; } - for (seq = 0; seq < DM_MAX_SEQ; seq++) { - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, seq, - false, &dup); + if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) && + (uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) { + /* dev_read_alias_highest_id() will return -1 if there no + * alias. Thus we can always add one. + */ + seq = dev_read_alias_highest_id(uc_drv->name) + 1; + } + + for (; seq < DM_MAX_SEQ; seq++) { + ret = uclass_find_device_by_seq(uc_drv->id, seq, false, &dup); if (ret == -ENODEV) break; if (ret)
If there are aliases for an uclass, set the base for the "dynamically" allocated numbers next to the highest alias. Please note, that this might lead to holes in the sequences, depending on the device tree. For example if there is only an alias "ethernet1", the next device seq number would be 2. In particular this fixes a problem with boards which are using ethernet aliases but also might have network add-in cards like the E1000. If the board is started with the add-in card and depending on the order of the drivers, the E1000 might occupy the first ethernet device and mess up all the hardware addresses, because the devices are now shifted by one. As a side effect, this should also make the following commits superfluous: - 7f3289bf6d ("dm: device: Request next sequence number") - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") Although I don't understand the root cause of the said problem. Thomas, Michal, could you please test this and then I'd add a second patch removing the old code. Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> Cc: Michal Simek <michal.simek@xilinx.com> Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/core/uclass.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)