Linux-USB Archive on
 help / color / Atom feed
From: Hans de Goede <>
To: Heikki Krogerus <>
Cc: MyungJoo Ham <>,
	Chanwoo Choi <>,
	Greg Kroah-Hartman <>,,
Subject: Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
Date: Thu, 10 Oct 2019 11:32:23 +0200
Message-ID: <> (raw)
In-Reply-To: <>


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 <>
>>> Tested-by: Hans de Goede <>
>>> 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?



*)  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.

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-08 12:25 Heikki Krogerus
2019-10-08 12:25 ` [PATCH v2 1/2] usb: roles: Add usb_role_switch_find_by_fwnode() Heikki Krogerus
2019-10-08 12:26 ` [PATCH v2 2/2] extcon: axp288: Remove the build-in connection description Heikki Krogerus
2019-10-08 13:59 ` [PATCH v2 0/2] extcon: axp288: Move to swnodes Hans de Goede
2019-10-08 14:01   ` Heikki Krogerus
2019-10-10  8:31     ` Heikki Krogerus
2019-10-10  9:32       ` Hans de Goede [this message]
2019-10-10 11:16         ` Heikki Krogerus
2019-10-10 11:58           ` Heikki Krogerus
2019-10-10 12:06             ` Hans de Goede
2019-11-04 13:09   ` Heikki Krogerus
2019-11-04 14:04     ` Greg Kroah-Hartman

Reply instructions:

You may reply publically 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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on

Archives are clonable:
	git clone --mirror linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ \
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone