Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
From: Hans de Goede
Date: Thu Oct 10 2019 - 05:32:32 EST
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@xxxxxxxxxx>
Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
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.