Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
Date: Thu, 10 Oct 2019 11:32:23 +0200
Message-ID: <7730d466-53bc-c14a-120f-dcb91de1e973@redhat.com> (raw)
In-Reply-To: <20191010083110.GA4981@kuha.fi.intel.com>

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.


  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:
  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=7730d466-53bc-c14a-120f-dcb91de1e973@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=cw00.choi@samsung.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=myungjoo.ham@samsung.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: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 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/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git