Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

Submitted by Gerd Hoffmann on Oct. 5, 2016, 6:34 a.m.

Details

Message ID 1475649297.25728.5.camel@redhat.com
State New
Headers show
Series "drm: Lobotomize set_busid nonsense for !pci drivers" ( rev: 4 ) in DRI devel

Not browsing as part of any series.

Commit Message

Gerd Hoffmann Oct. 5, 2016, 6:34 a.m.
On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > index a59d0e309bfc..1fcf739bf509 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> > struct virtio_device *vdev)
> >                 dev->pdev = pdev;
> >                 if (vga)
> >                         virtio_pci_kick_out_firmware_fb(pdev);
> > +
> > +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> > +               if (ret)
> > +                       goto err_free;
> >         }
> 
> Approach looks sensible to me, I'll give it a try ...

Well, dev_name() returns a string without the "pci:" prefix, we have to
add that to make things actually work, like this:

        ret = drm_dev_register(dev, 0);

And a partial revert of commit a742946a1ba57e24e8be205ea87224c05b38c380
to re-export drm_dev_set_unique().

Pushed to git://git.kraxel.org/linux drm-qemu

cheers,
  Gerd

Patch hide | download patch | download mbox

--- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
@@ -60,13 +60,22 @@  int drm_virtio_init(struct drm_driver *driver,
struct virtio_device *vdev)
 
        if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) {
                struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
+               const char *pname = dev_name(&pdev->dev);
                bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
+               char unique[20];
 
-               DRM_INFO("pci: %s detected\n",
-                        vga ? "virtio-vga" : "virtio-gpu-pci");
+               DRM_INFO("pci: %s detected at %s\n",
+                        vga ? "virtio-vga" : "virtio-gpu-pci",
+                        pname);
                dev->pdev = pdev;
                if (vga)
                        virtio_pci_kick_out_firmware_fb(pdev);
+
+               snprintf(unique, sizeof(unique), "pci:%s", pname);
+               ret = drm_dev_set_unique(dev, unique);
+               if (ret)
+                       goto err_free;
+
        }
 

Comments

On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote:
> On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > index a59d0e309bfc..1fcf739bf509 100644
> > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> > > struct virtio_device *vdev)
> > >                 dev->pdev = pdev;
> > >                 if (vga)
> > >                         virtio_pci_kick_out_firmware_fb(pdev);
> > > +
> > > +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> > > +               if (ret)
> > > +                       goto err_free;
> > >         }
> > 
> > Approach looks sensible to me, I'll give it a try ...
> 
> Well, dev_name() returns a string without the "pci:" prefix, we have to
> add that to make things actually work, like this:

Hm right, I missed that in digging through the piles of layers of struct
device naming.

> 
> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver,
> struct virtio_device *vdev)
>  
>         if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) {
>                 struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
> +               const char *pname = dev_name(&pdev->dev);
>                 bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> +               char unique[20];

Iirc we kfree this on unload. Needs to be a kasprintf I think.
>  
> -               DRM_INFO("pci: %s detected\n",
> -                        vga ? "virtio-vga" : "virtio-gpu-pci");
> +               DRM_INFO("pci: %s detected at %s\n",
> +                        vga ? "virtio-vga" : "virtio-gpu-pci",
> +                        pname);
>                 dev->pdev = pdev;
>                 if (vga)
>                         virtio_pci_kick_out_firmware_fb(pdev);
> +
> +               snprintf(unique, sizeof(unique), "pci:%s", pname);
> +               ret = drm_dev_set_unique(dev, unique);
> +               if (ret)
> +                       goto err_free;
> +

I think once we can nuke the pci_setbusid stuff we might want a
drm_dev_pci_set_unique, which takes the struct pci_device and wraps the
above magic. But that can wait, we still have a few years on the uabi
clock for that.

Strictly speaking this is an issue for virtgpu too, but since virtgpu is
newer than the last broken libdrm release I think we'll be fine.

With the kasprintf change above:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>         }
>  
>         ret = drm_dev_register(dev, 0);
> 
> And a partial revert of commit a742946a1ba57e24e8be205ea87224c05b38c380
> to re-export drm_dev_set_unique().
> 
> Pushed to git://git.kraxel.org/linux drm-qemu
> 
> cheers,
>   Gerd
>
On Mi, 2016-10-05 at 12:30 +0200, Daniel Vetter wrote:
> On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote:
> > On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > index a59d0e309bfc..1fcf739bf509 100644
> > > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> > > > struct virtio_device *vdev)
> > > >                 dev->pdev = pdev;
> > > >                 if (vga)
> > > >                         virtio_pci_kick_out_firmware_fb(pdev);
> > > > +
> > > > +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> > > > +               if (ret)
> > > > +                       goto err_free;
> > > >         }
> > > 
> > > Approach looks sensible to me, I'll give it a try ...
> > 
> > Well, dev_name() returns a string without the "pci:" prefix, we have to
> > add that to make things actually work, like this:
> 
> Hm right, I missed that in digging through the piles of layers of struct
> device naming.
> 
> > 
> > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver,
> > struct virtio_device *vdev)
> >  
> >         if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) {
> >                 struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
> > +               const char *pname = dev_name(&pdev->dev);
> >                 bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> > +               char unique[20];
> 
> Iirc we kfree this on unload. Needs to be a kasprintf I think.

drm_dev_set_unique() does this:

dev->unique = kstrdup(name, GFP_KERNEL);

So it should not matter at all where the caller stores name.
Or do you mean something else?

cheers,
  Gerd
On Wed, Oct 05, 2016 at 02:43:37PM +0200, Gerd Hoffmann wrote:
> On Mi, 2016-10-05 at 12:30 +0200, Daniel Vetter wrote:
> > On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote:
> > > On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote:
> > > >   Hi,
> > > > 
> > > > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > > index a59d0e309bfc..1fcf739bf509 100644
> > > > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> > > > > struct virtio_device *vdev)
> > > > >                 dev->pdev = pdev;
> > > > >                 if (vga)
> > > > >                         virtio_pci_kick_out_firmware_fb(pdev);
> > > > > +
> > > > > +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> > > > > +               if (ret)
> > > > > +                       goto err_free;
> > > > >         }
> > > > 
> > > > Approach looks sensible to me, I'll give it a try ...
> > > 
> > > Well, dev_name() returns a string without the "pci:" prefix, we have to
> > > add that to make things actually work, like this:
> > 
> > Hm right, I missed that in digging through the piles of layers of struct
> > device naming.
> > 
> > > 
> > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver,
> > > struct virtio_device *vdev)
> > >  
> > >         if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) {
> > >                 struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
> > > +               const char *pname = dev_name(&pdev->dev);
> > >                 bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> > > +               char unique[20];
> > 
> > Iirc we kfree this on unload. Needs to be a kasprintf I think.
> 
> drm_dev_set_unique() does this:
> 
> dev->unique = kstrdup(name, GFP_KERNEL);
> 
> So it should not matter at all where the caller stores name.
> Or do you mean something else?

Oh, missed that. r-b stands as-is.
-Daniel