| Message ID | 20191008122600.22340-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | extcon: axp288: Move to swnodes | expand |
Hi, On 08-10-2019 14:25, Heikki Krogerus wrote: > Hi Hans, > > Fixed the compiler warning in this version. No other changes. > > The original cover letter: > > That AXP288 extcon driver is the last that uses build-in connection > description. I'm replacing it with a code that finds the role mux > software node instead. > > I'm proposing also here a little helper > usb_role_switch_find_by_fwnode() that uses > class_find_device_by_fwnode() to find the role switches. Both patches look good to me and I can confirm that things still work as they should on a CHT device with an AXP288 PMIC, so for both: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Tested-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans p.s. I guess this means we can remove the build-in connection ( device_connection_add / remove) stuff now?
On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote: > Hi, > > On 08-10-2019 14:25, Heikki Krogerus wrote: > > Hi Hans, > > > > Fixed the compiler warning in this version. No other changes. > > > > The original cover letter: > > > > That AXP288 extcon driver is the last that uses build-in connection > > description. I'm replacing it with a code that finds the role mux > > software node instead. > > > > I'm proposing also here a little helper > > usb_role_switch_find_by_fwnode() that uses > > class_find_device_by_fwnode() to find the role switches. > > Both patches look good to me and I can confirm that things still > work as they should on a CHT device with an AXP288 PMIC, so for both: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Tested-by: Hans de Goede <hdegoede@redhat.com> > > Regards, > > Hans > > p.s. > > I guess this means we can remove the build-in connection ( > device_connection_add / remove) stuff now? Yes. I'll prepare separate patches for that. thanks,
On Tue, Oct 08, 2019 at 05:02:04PM +0300, Heikki Krogerus wrote: > On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote: > > Hi, > > > > On 08-10-2019 14:25, Heikki Krogerus wrote: > > > Hi Hans, > > > > > > Fixed the compiler warning in this version. No other changes. > > > > > > The original cover letter: > > > > > > That AXP288 extcon driver is the last that uses build-in connection > > > description. I'm replacing it with a code that finds the role mux > > > software node instead. > > > > > > I'm proposing also here a little helper > > > usb_role_switch_find_by_fwnode() that uses > > > class_find_device_by_fwnode() to find the role switches. > > > > Both patches look good to me and I can confirm that things still > > work as they should on a CHT device with an AXP288 PMIC, so for both: > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > > > Regards, > > > > Hans > > > > p.s. > > > > I guess this means we can remove the build-in connection ( > > device_connection_add / remove) stuff now? > > Yes. I'll prepare separate patches for that. Actually, maybe it would make sense to just remove it in this series. I'm attaching a patch that remove struct device_connection completely. Can you check if it makes sense to you? thanks,
Hi, On 10-10-2019 10:31, Heikki Krogerus wrote: > On Tue, Oct 08, 2019 at 05:02:04PM +0300, Heikki Krogerus wrote: >> On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote: >>> Hi, >>> >>> On 08-10-2019 14:25, Heikki Krogerus wrote: >>>> Hi Hans, >>>> >>>> Fixed the compiler warning in this version. No other changes. >>>> >>>> The original cover letter: >>>> >>>> That AXP288 extcon driver is the last that uses build-in connection >>>> description. I'm replacing it with a code that finds the role mux >>>> software node instead. >>>> >>>> I'm proposing also here a little helper >>>> usb_role_switch_find_by_fwnode() that uses >>>> class_find_device_by_fwnode() to find the role switches. >>> >>> Both patches look good to me and I can confirm that things still >>> work as they should on a CHT device with an AXP288 PMIC, so for both: >>> >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >>> Tested-by: Hans de Goede <hdegoede@redhat.com> >>> >>> Regards, >>> >>> Hans >>> >>> p.s. >>> >>> I guess this means we can remove the build-in connection ( >>> device_connection_add / remove) stuff now? >> >> Yes. I'll prepare separate patches for that. > > Actually, maybe it would make sense to just remove it in this series. > I'm attaching a patch that remove struct device_connection completely. > Can you check if it makes sense to you? This bit seems broken: static void * fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id, void *data, devcon_match_fn_t match) { - struct device_connection con = { .id = con_id }; struct fwnode_handle *ep; void *ret; fwnode_graph_for_each_endpoint(fwnode, ep) { - con.fwnode = fwnode_graph_get_remote_port_parent(ep); - if (!fwnode_device_is_available(con.fwnode)) + fwnode = fwnode_graph_get_remote_port_parent(ep); You are no replacing the passed in fwnode with the fwnode for the remote_port_parent and then the next time through the loop you will look at the wrong fwnode as the fwnode argument to fwnode_graph_for_each_endpoint() gets evaluated multiple times: #define fwnode_graph_for_each_endpoint(fwnode, child) \ for (child = NULL; \ (child = fwnode_graph_get_next_endpoint(fwnode, child)); ) ### And there is a similar problem here, where you again use the fwmode argument also as local variable in a loop where the function argument should be evaluated more then once: fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, void *data, devcon_match_fn_t match) { - struct device_connection con = { }; void *ret; int i; for (i = 0; ; i++) { - con.fwnode = fwnode_find_reference(fwnode, con_id, i); - if (IS_ERR(con.fwnode)) + fwnode = fwnode_find_reference(fwnode, con_id, i); ### And it seems that this bit where you introduce -EPROBE_DEFER is an unrelated behavior change? : +static void *generic_match(struct fwnode_handle *fwnode, const char *id, + void *data) { struct bus_type *bus; struct device *dev; + void *ret = NULL; for (bus = generic_match_buses[0]; bus; bus++) { - dev = bus_find_device_by_fwnode(bus, con->fwnode); - if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id))) - return dev; + dev = bus_find_device_by_fwnode(bus, fwnode); + if (dev) { + if (!strncmp(dev_name(dev), id, strlen(id))) + return dev; + ret = ERR_PTR(-EPROBE_DEFER); + } Note that the old generic_match code had: - if (con->fwnode) - return device_connection_fwnode_match(con); Which will simply always return either the dev or NULL, so as said this is a behavior change AFAICT. I've been trying to figure out what you are trying to do here and I found a troublesome path through the old code: 1. device_connection_find() gets called on a device with a fwnode 2. device_connection_find() calls device_connection_find_match() 3. device_connection_find_match() calls fwnode_connection_find_match() 4. fwnode_connection_find_match() calls fwnode_graph_devcon_match() this returns NULL 5. fwnode_connection_find_match() calls fwnode_devcon_match() 6. fwnode_devcon_match() creates a struct device_connection with just fwnode set, the rest is 0/NULL 7. fwnode_devcon_match() calls generic_match() with this struct 8. generic_match() calls device_connection_fwnode_match() because con->fwnode is set 9. device_connection_fwnode_match() does the following if a device is found: if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id))) return dev; but con->id is NULL here, so we have a NULL pointer deref here! We are currently not hitting this path because there are no callers of device_connection_find() all devcon users currently use device_connection_find_match() Note I believe the code after your patch still has this problem. Also doing the strcmp on the dev_name seems wrong? At least for the above code path, where fwnode_devcon_match() has already used / consumed the id. So at a minimum we need to add a id == NULL check to generic_match (*), but Since there are no users and the strcmp to to dev_name is weird, I believe that it would be better to just remove device_connection_find() (and generic_match, etc.) ? This could be a preparation patch for the patch you attached, this would simplify this patch a bit. ### If we end up with something like your suggested patch I think it might be good to do a follow up where device_connection_find_match callers simply call fwnode_connection_find_match directly and we remove device_connection_find_match ? Or maybe turn it into a static inline function? Regards, Hans *) Note that the typec related callers of device_connection_find_match() all 3 either already have an id == NULL check, or just ignore id completely.
On Thu, Oct 10, 2019 at 11:32:23AM +0200, Hans de Goede wrote: > Hi, > > On 10-10-2019 10:31, Heikki Krogerus wrote: > > On Tue, Oct 08, 2019 at 05:02:04PM +0300, Heikki Krogerus wrote: > > > On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 08-10-2019 14:25, Heikki Krogerus wrote: > > > > > Hi Hans, > > > > > > > > > > Fixed the compiler warning in this version. No other changes. > > > > > > > > > > The original cover letter: > > > > > > > > > > That AXP288 extcon driver is the last that uses build-in connection > > > > > description. I'm replacing it with a code that finds the role mux > > > > > software node instead. > > > > > > > > > > I'm proposing also here a little helper > > > > > usb_role_switch_find_by_fwnode() that uses > > > > > class_find_device_by_fwnode() to find the role switches. > > > > > > > > Both patches look good to me and I can confirm that things still > > > > work as they should on a CHT device with an AXP288 PMIC, so for both: > > > > > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > > > > > > > Regards, > > > > > > > > Hans > > > > > > > > p.s. > > > > > > > > I guess this means we can remove the build-in connection ( > > > > device_connection_add / remove) stuff now? > > > > > > Yes. I'll prepare separate patches for that. > > > > Actually, maybe it would make sense to just remove it in this series. > > I'm attaching a patch that remove struct device_connection completely. > > Can you check if it makes sense to you? > > This bit seems broken: > > static void * > fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > void *data, devcon_match_fn_t match) > { > - struct device_connection con = { .id = con_id }; > struct fwnode_handle *ep; > void *ret; > > fwnode_graph_for_each_endpoint(fwnode, ep) { > - con.fwnode = fwnode_graph_get_remote_port_parent(ep); > - if (!fwnode_device_is_available(con.fwnode)) > + fwnode = fwnode_graph_get_remote_port_parent(ep); > > You are no replacing the passed in fwnode with the fwnode for the > remote_port_parent and then the next time through the loop you will > look at the wrong fwnode as the fwnode argument to > fwnode_graph_for_each_endpoint() gets evaluated multiple times: > > #define fwnode_graph_for_each_endpoint(fwnode, child) \ > for (child = NULL; \ > (child = fwnode_graph_get_next_endpoint(fwnode, child)); ) > > ### > > And there is a similar problem here, where you again use the fwmode > argument also as local variable in a loop where the function > argument should be evaluated more then once: > > fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > void *data, devcon_match_fn_t match) > { > - struct device_connection con = { }; > void *ret; > int i; > > for (i = 0; ; i++) { > - con.fwnode = fwnode_find_reference(fwnode, con_id, i); > - if (IS_ERR(con.fwnode)) > + fwnode = fwnode_find_reference(fwnode, con_id, i); > > ### > > And it seems that this bit where you introduce -EPROBE_DEFER is an unrelated > behavior change? : > > +static void *generic_match(struct fwnode_handle *fwnode, const char *id, > + void *data) > { > struct bus_type *bus; > struct device *dev; > + void *ret = NULL; > > for (bus = generic_match_buses[0]; bus; bus++) { > - dev = bus_find_device_by_fwnode(bus, con->fwnode); > - if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id))) > - return dev; > + dev = bus_find_device_by_fwnode(bus, fwnode); > + if (dev) { > + if (!strncmp(dev_name(dev), id, strlen(id))) > + return dev; > + ret = ERR_PTR(-EPROBE_DEFER); > + } > > > Note that the old generic_match code had: > > - if (con->fwnode) > - return device_connection_fwnode_match(con); > > Which will simply always return either the dev or NULL, so as said this > is a behavior change AFAICT. > > I've been trying to figure out what you are trying to do here and > I found a troublesome path through the old code: > > 1. device_connection_find() gets called on a device with a fwnode > 2. device_connection_find() calls device_connection_find_match() > 3. device_connection_find_match() calls fwnode_connection_find_match() > 4. fwnode_connection_find_match() calls fwnode_graph_devcon_match() this returns NULL > 5. fwnode_connection_find_match() calls fwnode_devcon_match() > 6. fwnode_devcon_match() creates a struct device_connection with just fwnode set, the rest is 0/NULL > 7. fwnode_devcon_match() calls generic_match() with this struct > 8. generic_match() calls device_connection_fwnode_match() because con->fwnode is set > 9. device_connection_fwnode_match() does the following if a device is found: > if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id))) > return dev; > but con->id is NULL here, so we have a NULL pointer deref here! > > We are currently not hitting this path because there are no callers of > device_connection_find() all devcon users currently use device_connection_find_match() > > Note I believe the code after your patch still has this problem. Also doing > the strcmp on the dev_name seems wrong? At least for the above code path, where > fwnode_devcon_match() has already used / consumed the id. > > So at a minimum we need to add a id == NULL check to generic_match (*), but > Since there are no users and the strcmp to to dev_name is weird, I believe that > it would be better to just remove device_connection_find() (and generic_match, etc.) ? Yes! That makes sense to me. That function really serves no purpose. > This could be a preparation patch for the patch you attached, this would simplify > this patch a bit. > > ### > > If we end up with something like your suggested patch I think it might be good to > do a follow up where device_connection_find_match callers simply call > fwnode_connection_find_match directly and we remove device_connection_find_match ? > Or maybe turn it into a static inline function? Sure. I think it would make sense to move that fwnode_connection_find_match() function under drivers/base/property.c at the same time, and get rid of this devcon.c file completely. But before that, I would like to do a change to the software node support, if that's OK. I have to now add support for the "device graph" to the software nodes in any case (not because of this). I'm really not a fan of that "device graph" thing (I think it's over engineered) but if the software nodes support it, I think we probable need to always use it to describe these connections (because that's what DT uses). That would leave us with only one method of describing these connections (the device graph) compared to the three we have now (built-in, device graph and the "references"). In any case, do we leave this series as it is now, or should I add two patches to it - one that just removes device_connection_add/remove functions without any other changes, and another patch that removes that device_connection_find() function (together with generic_match etc.)? thanks,
On Thu, Oct 10, 2019 at 02:16:23PM +0300, Heikki Krogerus wrote: > In any case, do we leave this series as it is now, or should I add two > patches to it - one that just removes device_connection_add/remove > functions without any other changes, and another patch that removes > that device_connection_find() function (together with generic_match > etc.)? Forget about it. Let's leave this series as it is now. The device_connection_find() function we can remove separately. thanks,
Hi, On 10-10-2019 13:58, Heikki Krogerus wrote: > On Thu, Oct 10, 2019 at 02:16:23PM +0300, Heikki Krogerus wrote: >> In any case, do we leave this series as it is now, or should I add two >> patches to it - one that just removes device_connection_add/remove >> functions without any other changes, and another patch that removes >> that device_connection_find() function (together with generic_match >> etc.)? > > Forget about it. Let's leave this series as it is now. > > The device_connection_find() function we can remove separately. That sounds fine to me. Note as mentioned I would remove the device_connection_find() function before removing the builtin connection support, that will make the builtin connection support removal patch a bit cleaner. Regards, Hans
Hi Greg, On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote: > Hi, > > On 08-10-2019 14:25, Heikki Krogerus wrote: > > Hi Hans, > > > > Fixed the compiler warning in this version. No other changes. > > > > The original cover letter: > > > > That AXP288 extcon driver is the last that uses build-in connection > > description. I'm replacing it with a code that finds the role mux > > software node instead. > > > > I'm proposing also here a little helper > > usb_role_switch_find_by_fwnode() that uses > > class_find_device_by_fwnode() to find the role switches. > > Both patches look good to me and I can confirm that things still > work as they should on a CHT device with an AXP288 PMIC, so for both: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Tested-by: Hans de Goede <hdegoede@redhat.com> These two patches in this series are basically about the usb role API, so can you take them? thanks,
On Mon, Nov 04, 2019 at 03:09:04PM +0200, Heikki Krogerus wrote: > Hi Greg, > > On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote: > > Hi, > > > > On 08-10-2019 14:25, Heikki Krogerus wrote: > > > Hi Hans, > > > > > > Fixed the compiler warning in this version. No other changes. > > > > > > The original cover letter: > > > > > > That AXP288 extcon driver is the last that uses build-in connection > > > description. I'm replacing it with a code that finds the role mux > > > software node instead. > > > > > > I'm proposing also here a little helper > > > usb_role_switch_find_by_fwnode() that uses > > > class_find_device_by_fwnode() to find the role switches. > > > > Both patches look good to me and I can confirm that things still > > work as they should on a CHT device with an AXP288 PMIC, so for both: > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > These two patches in this series are basically about the usb role API, > so can you take them? Sure, will do that, thanks. greg k-h