[PATCH 0/3] drm: Store USB device in struct drm_device

[Hans de Goede]

Hi,

On 10/22/20 12:57 PM, Thomas Zimmermann wrote:
> Hi Hans
> 
> On 22.10.20 12:17, Hans de Goede wrote:
>> Hi,
>>
>> On 10/22/20 11:30 AM, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> On 22.10.20 11:20, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 10/21/20 3:07 PM, Thomas Zimmermann wrote:
>>>>> The drivers gm12u320 and udl operate on USB devices. They leave the
>>>>> PCI device in struct drm_device empty and store the USB device in their
>>>>> own driver structure.
>>>>>
>>>>> Fix this special case and save a few bytes by putting the USB device
>>>>> into an anonymous union with the PCI data. It's expected that DRM
>>>>> core and helpers only touch the PCI-device field for actual PCI devices.
>>>>>
>>>>> Thomas Zimmermann (3):
>>>>>   drm: Add reference to USB device to struct drm_device
>>>>>   drm/tiny/gm12u320: Store USB device in struct drm_device.udev
>>>>>   drm/udl: Store USB device in struct drm_device.udev
>>>>
>>>> This series looks good to me:
>>>>
>>>> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
>>>
>>> Thanks a lot. Following Daniel's request, I'll drop patch 1 and instead
>>> do an upcast from drm_device.dev to the USB device structure. The driver
>>> patches 2 and 3 will be slightly different. Unless you object, I''ll
>>> take the r-b into the new patches.
>>
>> I somehow missed Daniel's reply about this.
>>
>> With that said, hmm that is going to be an interesting up-cast, at least
>> for the gm12u320, that is going to look something like this:
>>
>> 	struct usb_device *udev = interface_to_usbdev(to_usb_interface(drm_dev->dev));
>>
>> (I wrote drm_dev instead of dev to make it more clear what is going on)
>>
>> For the DRM_DEV_ERROR() macro you can just use gm12u320->dev.dev ,
>> that will make the errors be printed with the in usb-interface device-name
>> as prefix instead of the usb-device device-name, but that is fine.
>>
>> I wonder of this is all worth it then though, just to save those few bytes ?
>>
> 
> Daniel and I briefly discussed on IRC if we could make drm_device.pdev
> (the PCI dev ) a legacy field in the longer term. All Linux devices
> would be retrieved from drm_device.dev then.

I see. Then I guess the fancy cast which I gave above is the right
way to move forward with this.

>> The first version made some sense since it made how drm devices with
>> usb resp. pci parents are handled consistent. Now it seems to make the code
>> somewhat harder to understand just to save the storage for a single pointer...
> 
> What's the implication of setting drm_device.dev to the value of struct
> usb_device.dev ? That's the device all the code interacts with.

USB drivers bind to USB interfaces, not to the USB device (the parent
of the interfaces). A single USB device may have multiple interfaces,
e.g. an USB headset may implement the USB audio class for the microphone
and headphones function, while having a second interface implementing the
HID interface for things like volume buttons, a play/pause button, etc.

So from the USB device model's pov making the USB device itself the parent
of the drm device is just wrong. I guess it may not cause much issues, but
it very much goes against how USB devices / interfaces are supposed to be
used inside the kernel.

So I guess we should just move forward with a v2 with the long / fancy casts.

As for keeping my Reviewed-by for that v2, yes that is fine.

Regards,

Hans