Re: [PATCH 10/18] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2333 bytes --]

On Tue, Apr 22, 2014 at 11:46:45AM +0200, Daniel Vetter wrote:
> On Thu, Apr 17, 2014 at 12:02:07AM +0200, Laurent Pinchart wrote:
> > And another comment...
> > 
> > On Friday 11 April 2014 23:36:07 Daniel Vetter wrote:
> > > To get rid of the dev->bus->get_irq callback we need to pass in the
> > > desired irq explicitly into drm_irq_install. To avoid having to do the
> > > same for drm_irq_unistall just track it internally. That leaves
> > > drivers with less room to botch things up.
> > > 
> > > v2: Add the hunk lost in an earlier patch to this one (Thierry).
> > > 
> > > Cc: Thierry Reding <thierry.reding@gmail.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
> > >  include/drm/drmP.h        |  2 ++
> > >  2 files changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index 330e85b19115..1c3b6229363d 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device
> > > *dev) */
> > >  int drm_irq_install(struct drm_device *dev)
> > >  {
> > > -	int ret;
> > > +	int ret, irq;
> > >  	unsigned long sh_flags = 0;
> > >  	char *irqname;
> > > 
> > > +	irq = drm_dev_to_irq(dev);
> > > +
> > >  	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> > >  		return -EINVAL;
> > > 
> > > -	if (drm_dev_to_irq(dev) == 0)
> > > +	if (irq == 0)
> > 
> > Isn't 0 a valid IRQ number ? Shouldn't you check for irq < 0 instead ? At 
> > least platform_get_irq() returns a negative error value on failure.
> 
> tbh I'm not really clear on how this works. If we want to change this then
> I think that should be a separate patch. Also it might be better to
> extract this check into drm_control, which is the only function that
> really needs it since it is called by userspace.
> 
> For all other places it is a simple driver bug and I'm not sure how much
> we should care - request_irq should catch any abuse already.
> 
> In any case this should be a separate patch.

I agree. This patch mostly mechanically replaces things and this bug did
already exist previously. So let's fix it separately (if at all).

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel