All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jarod Wilson <jarod@redhat.com>
To: Luis Henriques <luis.henriques@canonical.com>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH v2] [media] rc: Postpone ISR registration
Date: Fri, 20 Apr 2012 16:50:02 -0400	[thread overview]
Message-ID: <20120420205002.GD17452@redhat.com> (raw)
In-Reply-To: <1334871437-26514-1-git-send-email-luis.henriques@canonical.com>

On Thu, Apr 19, 2012 at 10:37:17PM +0100, Luis Henriques wrote:
> An early registration of an ISR was causing a crash to several users (for
> example, with the ite-cir driver: http://bugs.launchpad.net/bugs/972723).
> The reason was that IRQs were being triggered before a driver
> initialisation was completed.
> 
> This patch fixes this by moving the invocation to request_irq() and to
> request_region() to a later stage on the driver probe function.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> ---
>  drivers/media/rc/ene_ir.c      |   29 ++++++++++----------
>  drivers/media/rc/fintek-cir.c  |   17 ++++++------
>  drivers/media/rc/ite-cir.c     |   18 ++++++-------
>  drivers/media/rc/nuvoton-cir.c |   33 ++++++++++++-----------
>  drivers/media/rc/winbond-cir.c |   58 ++++++++++++++++++++--------------------
>  5 files changed, 79 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c
> index 860c112..b38c5c7 100644
> --- a/drivers/media/rc/ene_ir.c
> +++ b/drivers/media/rc/ene_ir.c
> @@ -1018,21 +1018,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  
>  	spin_lock_init(&dev->hw_lock);
>  
> -	/* claim the resources */
>  	error = -EBUSY;

In each of these, I believe the setting of retval to -EBUSY needs to be
moved as well, or you're passing along a success retval from another
operatoin between here and where the request_foo functions moved.


> -	dev->hw_io = pnp_port_start(pnp_dev, 0);
> -	if (!request_region(dev->hw_io, ENE_IO_SIZE, ENE_DRIVER_NAME)) {
> -		dev->hw_io = -1;
> -		dev->irq = -1;
> -		goto error;
> -	}
> -
> -	dev->irq = pnp_irq(pnp_dev, 0);
> -	if (request_irq(dev->irq, ene_isr,
> -			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev)) {
> -		dev->irq = -1;
> -		goto error;
> -	}
>  
>  	pnp_set_drvdata(pnp_dev, dev);
>  	dev->pnp_dev = pnp_dev;
> @@ -1086,6 +1072,21 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  	device_set_wakeup_capable(&pnp_dev->dev, true);
>  	device_set_wakeup_enable(&pnp_dev->dev, true);
>  
> +	/* claim the resources */
> +	dev->hw_io = pnp_port_start(pnp_dev, 0);
> +	if (!request_region(dev->hw_io, ENE_IO_SIZE, ENE_DRIVER_NAME)) {
> +		dev->hw_io = -1;
> +		dev->irq = -1;
> +		goto error;
> +	}
> +
> +	dev->irq = pnp_irq(pnp_dev, 0);
> +	if (request_irq(dev->irq, ene_isr,
> +			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev)) {
> +		dev->irq = -1;
> +		goto error;
> +	}
> +
>  	error = rc_register_device(rdev);
>  	if (error < 0)
>  		goto error;
> diff --git a/drivers/media/rc/fintek-cir.c b/drivers/media/rc/fintek-cir.c
> index 392d4be..c6273c5 100644
> --- a/drivers/media/rc/fintek-cir.c
> +++ b/drivers/media/rc/fintek-cir.c
> @@ -515,14 +515,6 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
>  	spin_lock_init(&fintek->fintek_lock);
>  
>  	ret = -EBUSY;
> -	/* now claim resources */
> -	if (!request_region(fintek->cir_addr,
> -			    fintek->cir_port_len, FINTEK_DRIVER_NAME))
> -		goto failure;
> -
> -	if (request_irq(fintek->cir_irq, fintek_cir_isr, IRQF_SHARED,
> -			FINTEK_DRIVER_NAME, (void *)fintek))
> -		goto failure;
>  
>  	pnp_set_drvdata(pdev, fintek);
>  	fintek->pdev = pdev;
> @@ -558,6 +550,15 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
>  	/* rx resolution is hardwired to 50us atm, 1, 25, 100 also possible */
>  	rdev->rx_resolution = US_TO_NS(CIR_SAMPLE_PERIOD);
>  
> +	/* now claim resources */
> +	if (!request_region(fintek->cir_addr,
> +			    fintek->cir_port_len, FINTEK_DRIVER_NAME))
> +		goto failure;
> +
> +	if (request_irq(fintek->cir_irq, fintek_cir_isr, IRQF_SHARED,
> +			FINTEK_DRIVER_NAME, (void *)fintek))
> +		goto failure;
> +
>  	ret = rc_register_device(rdev);
>  	if (ret)
>  		goto failure;
> diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
> index 682009d..d88b304 100644
> --- a/drivers/media/rc/ite-cir.c
> +++ b/drivers/media/rc/ite-cir.c
> @@ -1516,15 +1516,6 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
>  	init_ir_raw_event(&itdev->rawir);
>  
>  	ret = -EBUSY;
> -	/* now claim resources */
> -	if (!request_region(itdev->cir_addr,
> -				dev_desc->io_region_size, ITE_DRIVER_NAME))
> -		goto failure;
> -
> -	if (request_irq(itdev->cir_irq, ite_cir_isr, IRQF_SHARED,
> -			ITE_DRIVER_NAME, (void *)itdev))
> -		goto failure;
> -
>  	/* set driver data into the pnp device */
>  	pnp_set_drvdata(pdev, itdev);
>  	itdev->pdev = pdev;
> @@ -1600,6 +1591,15 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
>  	rdev->driver_name = ITE_DRIVER_NAME;
>  	rdev->map_name = RC_MAP_RC6_MCE;
>  
> +	/* now claim resources */
> +	if (!request_region(itdev->cir_addr,
> +				dev_desc->io_region_size, ITE_DRIVER_NAME))
> +		goto failure;
> +
> +	if (request_irq(itdev->cir_irq, ite_cir_isr, IRQF_SHARED,
> +			ITE_DRIVER_NAME, (void *)itdev))
> +		goto failure;
> +
>  	ret = rc_register_device(rdev);
>  	if (ret)
>  		goto failure;
> diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
> index 144f3f5..8afe549 100644
> --- a/drivers/media/rc/nuvoton-cir.c
> +++ b/drivers/media/rc/nuvoton-cir.c
> @@ -1022,22 +1022,6 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
>  	spin_lock_init(&nvt->tx.lock);
>  
>  	ret = -EBUSY;
> -	/* now claim resources */
> -	if (!request_region(nvt->cir_addr,
> -			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
> -		goto failure;
> -
> -	if (request_irq(nvt->cir_irq, nvt_cir_isr, IRQF_SHARED,
> -			NVT_DRIVER_NAME, (void *)nvt))
> -		goto failure;
> -
> -	if (!request_region(nvt->cir_wake_addr,
> -			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
> -		goto failure;
> -
> -	if (request_irq(nvt->cir_wake_irq, nvt_cir_wake_isr, IRQF_SHARED,
> -			NVT_DRIVER_NAME, (void *)nvt))
> -		goto failure;
>  
>  	pnp_set_drvdata(pdev, nvt);
>  	nvt->pdev = pdev;
> @@ -1085,6 +1069,23 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
>  	rdev->tx_resolution = XYZ;
>  #endif
>  
> +	/* now claim resources */
> +	if (!request_region(nvt->cir_addr,
> +			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
> +		goto failure;
> +
> +	if (request_irq(nvt->cir_irq, nvt_cir_isr, IRQF_SHARED,
> +			NVT_DRIVER_NAME, (void *)nvt))
> +		goto failure;
> +
> +	if (!request_region(nvt->cir_wake_addr,
> +			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
> +		goto failure;
> +
> +	if (request_irq(nvt->cir_wake_irq, nvt_cir_wake_isr, IRQF_SHARED,
> +			NVT_DRIVER_NAME, (void *)nvt))
> +		goto failure;
> +
>  	ret = rc_register_device(rdev);
>  	if (ret)
>  		goto failure;
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index b09c5fa..8e88c96 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -991,35 +991,6 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
>  		"(w: 0x%lX, e: 0x%lX, s: 0x%lX, i: %u)\n",
>  		data->wbase, data->ebase, data->sbase, data->irq);
>  
> -	if (!request_region(data->wbase, WAKEUP_IOMEM_LEN, DRVNAME)) {
> -		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
> -			data->wbase, data->wbase + WAKEUP_IOMEM_LEN - 1);
> -		err = -EBUSY;
> -		goto exit_free_data;
> -	}
> -
> -	if (!request_region(data->ebase, EHFUNC_IOMEM_LEN, DRVNAME)) {
> -		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
> -			data->ebase, data->ebase + EHFUNC_IOMEM_LEN - 1);
> -		err = -EBUSY;
> -		goto exit_release_wbase;
> -	}
> -
> -	if (!request_region(data->sbase, SP_IOMEM_LEN, DRVNAME)) {
> -		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
> -			data->sbase, data->sbase + SP_IOMEM_LEN - 1);
> -		err = -EBUSY;
> -		goto exit_release_ebase;
> -	}
> -
> -	err = request_irq(data->irq, wbcir_irq_handler,
> -			  IRQF_DISABLED, DRVNAME, device);
> -	if (err) {
> -		dev_err(dev, "Failed to claim IRQ %u\n", data->irq);
> -		err = -EBUSY;
> -		goto exit_release_sbase;
> -	}
> -
>  	led_trigger_register_simple("cir-tx", &data->txtrigger);
>  	if (!data->txtrigger) {
>  		err = -ENOMEM;
> @@ -1061,6 +1032,35 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
>  	data->dev->priv = data;
>  	data->dev->dev.parent = &device->dev;
>  
> +	if (!request_region(data->wbase, WAKEUP_IOMEM_LEN, DRVNAME)) {
> +		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
> +			data->wbase, data->wbase + WAKEUP_IOMEM_LEN - 1);
> +		err = -EBUSY;
> +		goto exit_free_data;
> +	}
> +
> +	if (!request_region(data->ebase, EHFUNC_IOMEM_LEN, DRVNAME)) {
> +		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
> +			data->ebase, data->ebase + EHFUNC_IOMEM_LEN - 1);
> +		err = -EBUSY;
> +		goto exit_release_wbase;
> +	}
> +
> +	if (!request_region(data->sbase, SP_IOMEM_LEN, DRVNAME)) {
> +		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
> +			data->sbase, data->sbase + SP_IOMEM_LEN - 1);
> +		err = -EBUSY;
> +		goto exit_release_ebase;
> +	}
> +
> +	err = request_irq(data->irq, wbcir_irq_handler,
> +			  IRQF_DISABLED, DRVNAME, device);
> +	if (err) {
> +		dev_err(dev, "Failed to claim IRQ %u\n", data->irq);
> +		err = -EBUSY;
> +		goto exit_release_sbase;
> +	}
> +
>  	err = rc_register_device(data->dev);
>  	if (err)
>  		goto exit_free_rc;
> -- 
> 1.7.9.5
> 

-- 
Jarod Wilson
jarod@redhat.com


  reply	other threads:[~2012-04-20 20:50 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-19 21:37 [PATCH v2] [media] rc: Postpone ISR registration Luis Henriques
2012-04-20 20:50 ` Jarod Wilson [this message]
2012-04-21 16:25   ` [PATCH v3] " Luis Henriques
2012-04-23 18:38     ` Jarod Wilson
2012-04-30 18:56       ` Luis Henriques

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120420205002.GD17452@redhat.com \
    --to=jarod@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=luis.henriques@canonical.com \
    --cc=mchehab@infradead.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.