On Fri, 4 May 2018 11:47:48 +0200
Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:
> > Hi Rob,
> >
> > On Thu, 3 May 2018 12:12:39 -0500
> > Rob Herring <robh+dt@xxxxxxxxxx> wrote:
> >
> > > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> > > <boris.brezillon@xxxxxxxxxxx> wrote:
> > > > The device might be described in the device tree but not connected to
> > > > the I2C bus. Update the status property so that the DRM panel logic
> > > > returns -ENODEV when someone tries to get the panel attached to this
> > > > DT node.
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> > > > ---
> > > > .../gpu/drm/panel/panel-raspberrypi-touchscreen.c | 35 ++++++++++++++++++++++
> > > > 1 file changed, 35 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > index 2c9c9722734f..b8fcb1acef75 100644
> > > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> > > > .get_modes = rpi_touchscreen_get_modes,
> > > > };
> > > >
> > > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> > > > +{
> > > > + struct property *newprop;
> > > > +
> > > > + newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > > + if (!newprop)
> > > > + return;
> > > > +
> > > > + newprop->name = kstrdup("status", GFP_KERNEL);
> > > > + if (!newprop->name)
> > > > + goto err;
> > > > +
> > > > + newprop->value = kstrdup("fail", GFP_KERNEL);
> > > > + if (!newprop->value)
> > > > + goto err;
> > > > +
> > > > + newprop->length = sizeof("fail");
> > > > +
> > > > + if (of_update_property(i2c->dev.of_node, newprop))
> > > > + goto err;
> > > > +
> > >
> > > As I mentioned on irc, can you make this a common DT function.
> >
> > Yep, will move that to drivers/of/base.c and make it generic.
> >
> > >
> > > I'm not sure if it matters that we set status to fail vs. disabled. I
> > > somewhat prefer the latter as we already have other cases and I'd
> > > rather the api not pass a string in. I can't think of any reason to
> > > distinguish the difference between fail and disabled.
> >
> > Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
> > and "fail" seemed like a good match for what we are trying to express
> > here: "we failed to communicate with the device in the probe function
> > and want to mark it unusable", which is a bit different from "the
> > device was explicitly disabled by the user".
> >
> > Anyway, if you think "disabled" is more appropriate, I'll use that.
> >
> > >
> > > > + /* We intentionally leak the memory we allocate here, because the new
> > > > + * OF property might live longer than the underlying dev, so no way
> > > > + * we can use devm_kzalloc() here.
> > > > + */
> > > > + return;
> > > > +
> > > > +err:
> > > > + kfree(newprop->value);
> > > > + kfree(newprop->name);
> > > > + kfree(newprop);
> > > > +}
> > > > +
> > > > static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > > const struct i2c_device_id *id)
> > > > {
> > > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > > >
> > > > ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> > > > if (ver < 0) {
> > > > + rpi_touchscreen_set_status_fail(i2c);
> > >
> > > I've thought some more about this and I still think this should be
> > > handled in the driver core or i2c core.
> > >
> > > The reason is simple. I think the state of the system should be the
> > > same after this as if you booted with 'status = "disabled"' for this
> > > node. And that means the device should be removed completely because
> > > we don't create struct device's for disabled nodes.
> >
> > That was my feeling to when first discussing the issue with Daniel and
> > Thierry on IRC, but after digging a bit in the code I'm no longer sure
> > this is a good idea. At least, I don't think basing the decision to
> > disable the device (or mark it unusable) based on the return value of
> > the probe function is a good idea.
>
> I'm not so sure about that. -ENODEV seems like a very suitable error
> code to base that decision on. A random sampling of a handful of drivers
> confirms that this is primarily used to report situations where it is
> impossible for the device to ever be probed successfully, so might as
> well just remove it.
It's not that easy. It has to be done from the I2C core since it's the
only one who can call device_unregister() and cleanup the other bits
associated with an I2C device (see i2c_unregister_device()). Now, the
i2c_driver->probe() function is called from a context where I'm almost
sure device_unregister() can't be called since we might still be in the
device_register() path. The solution would be to queue the
unregistration work to a workqueue, but I'm not even sure this is safe
to do that. What if the I2C adapter exposing the device is removed in
the meantime? Of course, all of this can be addressed, I'm just
wondering if it's really worth the trouble (we're likely to introduce
new races or other kind of bugs while doing that), especially since
placing the device in a "fail" state and still keeping it around would
solve the problem without requiring all the extra cleanup we're talking
about here.
>
> At the very least I think it is worth proposing the patch and let Greg
> and others weigh in.
Well, if it was an easy thing to do, I guess I would have gone for this
approach from the beginning, but I fear doing that will be much more
complicated than we think it is (maybe I'm wrong).
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
