Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Fri, 4 May 2018 16:24:04 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Fri, 4 May 2018 16:20:17 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, May 04, 2018 at 02:17:49PM +0200, Boris Brezillon wrote:  
> > > On Fri, 4 May 2018 11:47:48 +0200
> > > Thierry Reding <thierry.reding@gmail.com> 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@kernel.org> wrote:
> > > > >       
> > > > > > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> > > > > > <boris.brezillon@bootlin.com> 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@bootlin.com>
> > > > > > > ---
> > > > > > >  .../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.    
> > 
> > I think you have to put the device status into "fail" immediately,
> > otherwise there's a race with deferred probing. Scenario:
> > 
> > 1. vc4 loads, panel isn't there yet -> EPROBE_DEFER.
> > 2. rpi driver loads, notices panel isn't there, returns -ENODEV
> > 3. i2c core fires off the worker and finishes it's ->probe callback.
> > 4. device core starts a reprobe trigger
> > 5. vc4 gets loaded, does the of_device_is_available check, but since
> > that's not yet update it doesn't get the ENODEV, but still EPROBE_DEFER.
> > 6. i2c worker disables the device and unregisters it.
> >   
> > -> vc4 fails to load since nothing triggers another reprobe anymore.    
> > 
> > At least afaics device removal does not trigger a reprobe.  
> 
> Yep, you're correct. See, one more reason to keep the logic simple and
> let each driver change the status prop in their ->probe() function.

Hm, actually it does not work even with my solution because the only
thing that forces a new attempt on all deferred-probe devices is when a
new device is bound to a driver, which will not happen if the rpi-panel
->probe() function returns -ENODEV.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel