Sorry Lukas, for forgetting these comments.

I'll take these comments to this thread as the other thread is a dead
end. I think the drm_panel_attach() (in this patch) is a better place to
add the device link than panel_bridge_attach() (the other patch).

On 21/02/18 09:52, Lukas Wunner wrote:
> On Wed, Feb 21, 2018 at 12:21:50AM +0200, Jyri Sarha wrote:
>> @@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge
>>      struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>      drm_panel_detach(panel_bridge->panel);
>> +
>> +    device_link_del(panel_bridge->link);
> No, you've set the DL_FLAG_AUTOREMOVE flag, so you'll end up removing
> the link twice, which oopses.
> It's either DL_FLAG_AUTOREMOVE or device_link_del(), not both.

Oh yes. I'll drop the device_link_del(). Now reading the code more
carefully, the link del from the driver is obviously redundant. However,
it does not cause a crash if the link is only deleted from the consumer

There is still one argument for having the link del in panel detach
however: If some drm device would behave more dynamically, and could
detach a panel on the fly but stay operational still, then it would be
correct to call link del from the drm driver. But if we ever have such a
device, we can solve that issue separately.

>> +static int panel_bridge_link_to_master(struct panel_bridge
>> +{
>> +    struct device *mdev = panel_bridge->>dev;
>> +    struct device *pdev = panel_bridge->panel->dev;
>> +    u32 flags = DL_FLAG_AUTOREMOVE;
>> +
>> +    panel_bridge->link = device_link_add(mdev, pdev, flags);
>> +    if (!panel_bridge->link) {
>> +            dev_err(pdev, "failed to link panel %s to %s\n",
>> +                    dev_name(pdev), dev_name(mdev));
> You're printing two instances of pdev's name in the log message,
> one should be sufficient.
Oh yes, I'll drop the panel device name from the format string.

> Also, you've mixed up the order: mdev is the consumer, pdev the
> supplier.
Yes, I noticed that myself. The new patch does not have this problem.

> (Bikeshed:  The DL_FLAG_AUTOREMOVE would still fit within 80 chars
> on the line with device_link_add() and the flags variable wouldn't
> have to be declared then.  Your call.)

This time I need the flags for having the line under 80 chars.

> Thanks,
> Lukas
Best regards,

On 23/02/18 08:58, Lukas Wunner wrote:
> On Thu, Feb 22, 2018 at 07:46:27PM +0200, Jyri Sarha wrote:
>> Add device_link from panel device (supplier) to drm device (consumer)
>> with DL_FLAG_AUTOREMOVE when drm_panel_attach() is called. Currently
>> the master drm driver is not protected against the attached. The
>> device_link with DL_FLAG_AUTOREMOVE should make sure the drm device is
>> unbound before the panel driver becomes unavailable.
>> The device_link is removed when drm_panel_detach() is called. The
>> drm_panel_detach() should be called by the panel driver it self when
>> it is removed. Otherwise the both driver are racing to delete the same
>> link.
> Unfortunately you haven't addressed any of my comments on the previous
> version of this patch:
> Please respin with my comments addressed.  Thanks,
> Lukas

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
dri-devel mailing list

Reply via email to