Re: [PATCH] drivers: uio_dmem_genirq: Fix memory leak in uio_dmem_genirq_probe()
On Thu, May 22, 2014 at 09:46:12AM +0900, Daeseok Youn wrote:
> When platform_get_irq() is failed after "priv" allocated,
> it need to free "priv". But the label of bad0 doesn't try
> to free about "priv". So this patch changes that lable to "bad1".
> But "bad1" has pm_runtime_disable() call, this function should
> be called when uio_register_device() is failed. So it is moved
> into handling error for uio_register_device().
Looks alright to me. Thanks for your contribution.
Signed-off-by: Hans J. Koch
>
> Signed-off-by: Daeseok Youn
> ---
> drivers/uio/uio_dmem_genirq.c |4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
> index 1270f3b..8d0bba4 100644
> --- a/drivers/uio/uio_dmem_genirq.c
> +++ b/drivers/uio/uio_dmem_genirq.c
> @@ -204,7 +204,7 @@ static int uio_dmem_genirq_probe(struct platform_device
> *pdev)
> ret = platform_get_irq(pdev, 0);
> if (ret < 0) {
> dev_err(>dev, "failed to get IRQ\n");
> - goto bad0;
> + goto bad1;
> }
> uioinfo->irq = ret;
> }
> @@ -275,6 +275,7 @@ static int uio_dmem_genirq_probe(struct platform_device
> *pdev)
> ret = uio_register_device(>dev, priv->uioinfo);
> if (ret) {
> dev_err(>dev, "unable to register uio device\n");
> + pm_runtime_disable(>dev);
> goto bad1;
> }
>
> @@ -282,7 +283,6 @@ static int uio_dmem_genirq_probe(struct platform_device
> *pdev)
> return 0;
> bad1:
> kfree(priv);
> - pm_runtime_disable(>dev);
> bad0:
> /* kfree uioinfo for OF */
> if (pdev->dev.of_node)
> --
> 1.7.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: uio_dmem_genirq: Fix memory leak in uio_dmem_genirq_probe()
On Thu, May 22, 2014 at 09:46:12AM +0900, Daeseok Youn wrote:
When platform_get_irq() is failed after priv allocated,
it need to free priv. But the label of bad0 doesn't try
to free about priv. So this patch changes that lable to bad1.
But bad1 has pm_runtime_disable() call, this function should
be called when uio_register_device() is failed. So it is moved
into handling error for uio_register_device().
Looks alright to me. Thanks for your contribution.
Signed-off-by: Hans J. Koch h...@hansjkoch.de
Signed-off-by: Daeseok Youn daeseok.y...@gmail.com
---
drivers/uio/uio_dmem_genirq.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
index 1270f3b..8d0bba4 100644
--- a/drivers/uio/uio_dmem_genirq.c
+++ b/drivers/uio/uio_dmem_genirq.c
@@ -204,7 +204,7 @@ static int uio_dmem_genirq_probe(struct platform_device
*pdev)
ret = platform_get_irq(pdev, 0);
if (ret 0) {
dev_err(pdev-dev, failed to get IRQ\n);
- goto bad0;
+ goto bad1;
}
uioinfo-irq = ret;
}
@@ -275,6 +275,7 @@ static int uio_dmem_genirq_probe(struct platform_device
*pdev)
ret = uio_register_device(pdev-dev, priv-uioinfo);
if (ret) {
dev_err(pdev-dev, unable to register uio device\n);
+ pm_runtime_disable(pdev-dev);
goto bad1;
}
@@ -282,7 +283,6 @@ static int uio_dmem_genirq_probe(struct platform_device
*pdev)
return 0;
bad1:
kfree(priv);
- pm_runtime_disable(pdev-dev);
bad0:
/* kfree uioinfo for OF */
if (pdev-dev.of_node)
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MAINTAINERS: Remove Hans J. Koch entries
On Thu, Jun 20, 2013 at 09:20:27AM -0700, Joe Perches wrote: > On Thu, 2013-06-20 at 18:01 +0200, Pavel Machek wrote: > > Anyway, Hans disappeared, so Greg takes patches, again. I wasn't able to take part in kernel development because I was heavily involved in hardware development project. I always thought it would be just a few more days, but then it became half a year. I know I should have given an explanation and didn't. Sorry for that. > > Maybe this is appropriate then. > > Signed-off-by: Joe Perches > --- > diff --git a/MAINTAINERS b/MAINTAINERS > index 69fea4f..dc9d04a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5253,9 +5253,8 @@ F: Documentation/hwmon/max16065 > F: drivers/hwmon/max16065.c > > MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER > -M: "Hans J. Koch" > L: lm-sens...@lm-sensors.org > -S: Maintained > +S: Orphan > F: Documentation/hwmon/max6650 > F: drivers/hwmon/max6650.c ACK to that one. It was never my idea to have a MAINTAINERS entry for that. Jean Delvare suggested it, so it came in. The MAX6650 was in a project I was working on 6 years ago, and I wrote the driver at that time. Meanwhile, I don't even have hardware with a MAX6650 anymore. Does each little driver really need a MAINTAINERS entry? In my opinion, it should only be there if it is clear that it's not just a short project work. > > @@ -8795,7 +8794,6 @@ F: fs/hostfs/ > F: fs/hppfs/ > > USERSPACE I/O (UIO) > -M: "Hans J. Koch" > M: Greg Kroah-Hartman > S: Maintained > F: Documentation/DocBook/uio-howto.tmpl Well, you can do that if you want. But I still feel associated with UIO and sincerely hope I'm back again to fulfill my duties as a maintainer. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MAINTAINERS: Remove Hans J. Koch entries
On Thu, Jun 20, 2013 at 09:20:27AM -0700, Joe Perches wrote: On Thu, 2013-06-20 at 18:01 +0200, Pavel Machek wrote: Anyway, Hans disappeared, so Greg takes patches, again. I wasn't able to take part in kernel development because I was heavily involved in hardware development project. I always thought it would be just a few more days, but then it became half a year. I know I should have given an explanation and didn't. Sorry for that. Maybe this is appropriate then. Signed-off-by: Joe Perches j...@perches.com --- diff --git a/MAINTAINERS b/MAINTAINERS index 69fea4f..dc9d04a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5253,9 +5253,8 @@ F: Documentation/hwmon/max16065 F: drivers/hwmon/max16065.c MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER -M: Hans J. Koch h...@hansjkoch.de L: lm-sens...@lm-sensors.org -S: Maintained +S: Orphan F: Documentation/hwmon/max6650 F: drivers/hwmon/max6650.c ACK to that one. It was never my idea to have a MAINTAINERS entry for that. Jean Delvare suggested it, so it came in. The MAX6650 was in a project I was working on 6 years ago, and I wrote the driver at that time. Meanwhile, I don't even have hardware with a MAX6650 anymore. Does each little driver really need a MAINTAINERS entry? In my opinion, it should only be there if it is clear that it's not just a short project work. @@ -8795,7 +8794,6 @@ F: fs/hostfs/ F: fs/hppfs/ USERSPACE I/O (UIO) -M: Hans J. Koch h...@hansjkoch.de M: Greg Kroah-Hartman gre...@linuxfoundation.org S: Maintained F: Documentation/DocBook/uio-howto.tmpl Well, you can do that if you want. But I still feel associated with UIO and sincerely hope I'm back again to fulfill my duties as a maintainer. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/2] New driver: Xillybus generic interface for FPGA (programmable logic)
On Fri, May 17, 2013 at 05:53:39PM +0300, Eli Billauer wrote: > This is the documentation for the Xillybus driver. It's intended for kernel > maintainers, since actual users will most likely rely on other documentation. > > It has turned out somewhat tricky to put Xillybus in one of the existing > categories of device drivers. I placed it under drivers/uio, mainly because it > shares the spirit of a generic driver allowing userspace programs to talk > directly with hardware. Unlike the classic UIO devices, Xillybus' char files > shouldn't be memory mapped, but rather used in the regular manner as streams > of data. In that case it simply isn't a UIO device. > > This driver was previously posted under drivers/misc/. The misc device > framework turned out to be inadequate, as the driver may create more than 100 > device files in some cases. And that is a problem? If yes, try something else like sysfs. From your description, this is a drivers/misc device for me. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/2] New driver: Xillybus generic interface for FPGA (programmable logic)
On Fri, May 17, 2013 at 05:53:39PM +0300, Eli Billauer wrote: This is the documentation for the Xillybus driver. It's intended for kernel maintainers, since actual users will most likely rely on other documentation. It has turned out somewhat tricky to put Xillybus in one of the existing categories of device drivers. I placed it under drivers/uio, mainly because it shares the spirit of a generic driver allowing userspace programs to talk directly with hardware. Unlike the classic UIO devices, Xillybus' char files shouldn't be memory mapped, but rather used in the regular manner as streams of data. In that case it simply isn't a UIO device. This driver was previously posted under drivers/misc/. The misc device framework turned out to be inadequate, as the driver may create more than 100 device files in some cases. And that is a problem? If yes, try something else like sysfs. From your description, this is a drivers/misc device for me. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/1] uio.c: solve memory leak
On Fri, Jan 18, 2013 at 02:00:50PM -0800, Greg Kroah-Hartman wrote: > On Fri, Jan 18, 2013 at 10:05:45PM +0100, Cong Ding wrote: > > On Tue, Dec 11, 2012 at 2:21 AM, Hans J. Koch wrote: > > > On Thu, Nov 29, 2012 at 05:40:00PM +, Cong Ding wrote: > > >> In version 1, I forgot to modify the same bug in the first loop. > > >> > > >> we have to call kobject_put() to clean up the kobject after function > > >> kobject_init(), kobject_add(), or kobject_uevent() is called. > > >> > > >> Signed-off-by: Cong Ding > > > > > > Signed-off-by: "Hans J. Koch" > > > > Hi Greg, is this patch stil in the queue? > > Hans is queueing up UIO patches to send to me, I'm waiting for him to > send them as I don't have any in my trees. I'll set that up tonight or tomorrow. Sorry, I was delayed by illness and a lot of other work. Thanks, Hans > > thanks, > > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/1] uio.c: solve memory leak
On Fri, Jan 18, 2013 at 02:00:50PM -0800, Greg Kroah-Hartman wrote: On Fri, Jan 18, 2013 at 10:05:45PM +0100, Cong Ding wrote: On Tue, Dec 11, 2012 at 2:21 AM, Hans J. Koch h...@hansjkoch.de wrote: On Thu, Nov 29, 2012 at 05:40:00PM +, Cong Ding wrote: In version 1, I forgot to modify the same bug in the first loop. we have to call kobject_put() to clean up the kobject after function kobject_init(), kobject_add(), or kobject_uevent() is called. Signed-off-by: Cong Ding ding...@gmail.com Signed-off-by: Hans J. Koch h...@hansjkoch.de Hi Greg, is this patch stil in the queue? Hans is queueing up UIO patches to send to me, I'm waiting for him to send them as I don't have any in my trees. I'll set that up tonight or tomorrow. Sorry, I was delayed by illness and a lot of other work. Thanks, Hans thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] Fix memory freeing issues
On Fri, Dec 14, 2012 at 11:33:50AM +0200, Vitalii Demianets wrote: > > Hans, why do you want to put in this patch, which is dealing with > memory-freeing issues only, completely unrelated functional changes? Because during review of your patch we happened to find another issue a few lines up and down. Why not fix it on the way? What I'd like is simply [PATCH] Fix uio_pdrv_genirq issues If you like, make it two patches, one with your memory-freeing issue and one "Remove irq tracking" or something like that. That's just three or four lines difference, I'd even accept it if it were only one patch. I don't want to fix one thing now and leave the other one unresolved. That would just be a waste of time. To be clear, I have no objections regarding your memory freeing ideas. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] Fix memory freeing issues
On Fri, Dec 14, 2012 at 11:33:50AM +0200, Vitalii Demianets wrote: Hans, why do you want to put in this patch, which is dealing with memory-freeing issues only, completely unrelated functional changes? Because during review of your patch we happened to find another issue a few lines up and down. Why not fix it on the way? What I'd like is simply [PATCH] Fix uio_pdrv_genirq issues If you like, make it two patches, one with your memory-freeing issue and one Remove irq tracking or something like that. That's just three or four lines difference, I'd even accept it if it were only one patch. I don't want to fix one thing now and leave the other one unresolved. That would just be a waste of time. To be clear, I have no objections regarding your memory freeing ideas. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[GIT PULL] UIO changes for v3.8
Hi Greg, my new UIO tree is based on your current char-misc tree. Please pull the following two patches. Thanks, Hans The following changes since commit 1ebaf4f4e6912199f8a4e30ba3ab55da2b71bcdf: Merge branch 'x86-timers-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip (2012-12-11 20:01:33 -0800) are available in the git repository at: git://hansjkoch.de:/home/repos/git/char-misc.git uio-devel for you to fetch changes up to d91796fe825b7ebd8ed4150c6886faeb7c3982b1: uio: fix possible memory leak in uio.c (2012-12-13 18:55:59 +0100) Cong Ding (1): uio: fix possible memory leak in uio.c Hans J. Koch (1): uio: Fix warning: 'ret' might be used uninitialized drivers/uio/uio.c | 18 -- 1 files changed, 12 insertions(+), 6 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] Fix memory freeing issues
On Mon, Dec 10, 2012 at 11:44:45AM +0200, Vitalii Demianets wrote:
> 1. uioinfo was kfreed based on the presence of pdev->dev.of_node, which was
> obviously wrong and unrelated to the fact if uioinfo was allocated statically
> or dynamically. This patch introduces new flag which clearly shows if uioinfo
> was allocated dynamically and kfrees uioinfo based on that flag;
> 2. Fix: priv data was not freed in case platform_get_irq() failed. As it was
> caused mainly by improper exit labels naming, labels were renamed too;
> 3. The case of uioinfo AND pdev->dev.of_node both NULL (not initialized
> in platform data) was not treated properly.
>
> Signed-off-by: Vitalii Demianets
> ---
> v2: Constants naming style
> v3: Comments: better wording in comment accompanying flags initialization &
> removed obsoleted OF comment
That comment is obsolete as well.
>
> ---
> drivers/uio/uio_pdrv_genirq.c | 47
> 1 files changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 42202cd..cc5233b 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -30,6 +30,11 @@
>
> #define DRIVER_NAME "uio_pdrv_genirq"
>
> +enum {
> + UIO_IRQ_DISABLED = 0,
> + UIO_INFO_ALLOCED = 1,
> +};
We don't need these flags.
> +
> struct uio_pdrv_genirq_platdata {
> struct uio_info *uioinfo;
> spinlock_t lock;
> @@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct
> uio_info *dev_info)
>* remember the state so we can allow user space to enable it later.
>*/
>
> - if (!test_and_set_bit(0, >flags))
> + if (!test_and_set_bit(UIO_IRQ_DISABLED, >flags))
Remove the "if" completely, we need to disable the irq unconditionally.
> disable_irq_nosync(irq);
>
> return IRQ_HANDLED;
> @@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info
> *dev_info, s32 irq_on)
>
> spin_lock_irqsave(>lock, flags);
> if (irq_on) {
> - if (test_and_clear_bit(0, >flags))
> + if (test_and_clear_bit(UIO_IRQ_DISABLED, >flags))
> enable_irq(dev_info->irq);
> } else {
> - if (!test_and_set_bit(0, >flags))
> + if (!test_and_set_bit(UIO_IRQ_DISABLED, >flags))
> disable_irq(dev_info->irq);
Same here: irqcontrol has to enable/disable the irq unconditionally.
> }
> spin_unlock_irqrestore(>lock, flags);
> @@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> struct uio_mem *uiomem;
> int ret = -EINVAL;
> int i;
> + bool uioinfo_alloced = false;
>
> - if (!uioinfo) {
> + if (!uioinfo && pdev->dev.of_node) {
> int irq;
>
> /* alloc uioinfo for one device */
> @@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> if (!uioinfo) {
> ret = -ENOMEM;
> dev_err(>dev, "unable to kmalloc\n");
> - goto bad2;
> + goto out;
> }
> uioinfo->name = pdev->dev.of_node->name;
> uioinfo->version = "devicetree";
> + uioinfo_alloced = true;
>
> /* Multiple IRQs are not supported */
> irq = platform_get_irq(pdev, 0);
> @@ -125,32 +132,35 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
>
> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> dev_err(>dev, "missing platform_data\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> if (uioinfo->handler || uioinfo->irqcontrol ||
> uioinfo->irq_flags & IRQF_SHARED) {
> dev_err(>dev, "interrupt configuration error\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> if (!priv) {
> ret = -ENOMEM;
> dev_err(>dev, "unable to kmalloc\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv->uioinfo = uioinfo;
> spin_lock_init(>lock);
> - priv->flags = 0; /* interrupt is enabled to begin with */
> + /* Interrupt is enabled to begin with,
> + * that's why UIO_IRQ_DISABLED flag is not initially set.
> + */
> + priv->flags = uioinfo_alloced ? BIT_MASK(UIO_INFO_ALLOCED) : 0;
> priv->pdev = pdev;
>
> if (!uioinfo->irq) {
> ret = platform_get_irq(pdev, 0);
> if (ret < 0) {
> dev_err(>dev, "failed to get IRQ\n");
> - goto bad0;
> + goto out_priv;
> }
> uioinfo->irq = ret;
> }
> @@ -205,19 +215,19 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Thu, Dec 13, 2012 at 07:23:21PM +0200, Vitalii Demianets wrote: > On Thursday 13 December 2012 19:11:09 Hans J. Koch wrote: > > On Tue, Dec 11, 2012 at 12:47:35PM +0200, Vitalii Demianets wrote: > > > Please, review the v3 of "Fix memory freeing issues" patch (first in the > > > series I posted yesterday) and ignore the second, as we haven't agreed > > > on it. > > > > I can't find a v3. Please resend it. > > > > I've posted v3 as a [PATCH 1/2 v3] in series: > http://www.spinics.net/lists/kernel/msg1450101.html > > You were CC-ed. If for some reason you didn't get it in your mailbox, I'll > resend. OK, found it, sorry. I ignored that one because it does the same flag-testing stuff. It is unnecessary and only tries to fix userspace stupidity in the kernel. I won't buy that one, and I already gave an explanation why. I won't take it just because you disagree with my opinion. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Tue, Dec 11, 2012 at 12:47:35PM +0200, Vitalii Demianets wrote: > > Please, review the v3 of "Fix memory freeing issues" patch (first in the > series I posted yesterday) and ignore the second, as we haven't agreed > on it. > I can't find a v3. Please resend it. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Tue, Dec 11, 2012 at 12:47:35PM +0200, Vitalii Demianets wrote: Please, review the v3 of Fix memory freeing issues patch (first in the series I posted yesterday) and ignore the second, as we haven't agreed on it. I can't find a v3. Please resend it. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Thu, Dec 13, 2012 at 07:23:21PM +0200, Vitalii Demianets wrote: On Thursday 13 December 2012 19:11:09 Hans J. Koch wrote: On Tue, Dec 11, 2012 at 12:47:35PM +0200, Vitalii Demianets wrote: Please, review the v3 of Fix memory freeing issues patch (first in the series I posted yesterday) and ignore the second, as we haven't agreed on it. I can't find a v3. Please resend it. I've posted v3 as a [PATCH 1/2 v3] in series: http://www.spinics.net/lists/kernel/msg1450101.html You were CC-ed. If for some reason you didn't get it in your mailbox, I'll resend. OK, found it, sorry. I ignored that one because it does the same flag-testing stuff. It is unnecessary and only tries to fix userspace stupidity in the kernel. I won't buy that one, and I already gave an explanation why. I won't take it just because you disagree with my opinion. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] Fix memory freeing issues
On Mon, Dec 10, 2012 at 11:44:45AM +0200, Vitalii Demianets wrote:
1. uioinfo was kfreed based on the presence of pdev-dev.of_node, which was
obviously wrong and unrelated to the fact if uioinfo was allocated statically
or dynamically. This patch introduces new flag which clearly shows if uioinfo
was allocated dynamically and kfrees uioinfo based on that flag;
2. Fix: priv data was not freed in case platform_get_irq() failed. As it was
caused mainly by improper exit labels naming, labels were renamed too;
3. The case of uioinfo AND pdev-dev.of_node both NULL (not initialized
in platform data) was not treated properly.
Signed-off-by: Vitalii Demianets vi...@nppfactor.kiev.ua
---
v2: Constants naming style
v3: Comments: better wording in comment accompanying flags initialization
removed obsoleted OF comment
That comment is obsolete as well.
---
drivers/uio/uio_pdrv_genirq.c | 47
1 files changed, 28 insertions(+), 19 deletions(-)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..cc5233b 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -30,6 +30,11 @@
#define DRIVER_NAME uio_pdrv_genirq
+enum {
+ UIO_IRQ_DISABLED = 0,
+ UIO_INFO_ALLOCED = 1,
+};
We don't need these flags.
+
struct uio_pdrv_genirq_platdata {
struct uio_info *uioinfo;
spinlock_t lock;
@@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct
uio_info *dev_info)
* remember the state so we can allow user space to enable it later.
*/
- if (!test_and_set_bit(0, priv-flags))
+ if (!test_and_set_bit(UIO_IRQ_DISABLED, priv-flags))
Remove the if completely, we need to disable the irq unconditionally.
disable_irq_nosync(irq);
return IRQ_HANDLED;
@@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info
*dev_info, s32 irq_on)
spin_lock_irqsave(priv-lock, flags);
if (irq_on) {
- if (test_and_clear_bit(0, priv-flags))
+ if (test_and_clear_bit(UIO_IRQ_DISABLED, priv-flags))
enable_irq(dev_info-irq);
} else {
- if (!test_and_set_bit(0, priv-flags))
+ if (!test_and_set_bit(UIO_IRQ_DISABLED, priv-flags))
disable_irq(dev_info-irq);
Same here: irqcontrol has to enable/disable the irq unconditionally.
}
spin_unlock_irqrestore(priv-lock, flags);
@@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
struct uio_mem *uiomem;
int ret = -EINVAL;
int i;
+ bool uioinfo_alloced = false;
- if (!uioinfo) {
+ if (!uioinfo pdev-dev.of_node) {
int irq;
/* alloc uioinfo for one device */
@@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
if (!uioinfo) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad2;
+ goto out;
}
uioinfo-name = pdev-dev.of_node-name;
uioinfo-version = devicetree;
+ uioinfo_alloced = true;
/* Multiple IRQs are not supported */
irq = platform_get_irq(pdev, 0);
@@ -125,32 +132,35 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
if (!uioinfo || !uioinfo-name || !uioinfo-version) {
dev_err(pdev-dev, missing platform_data\n);
- goto bad0;
+ goto out_uioinfo;
}
if (uioinfo-handler || uioinfo-irqcontrol ||
uioinfo-irq_flags IRQF_SHARED) {
dev_err(pdev-dev, interrupt configuration error\n);
- goto bad0;
+ goto out_uioinfo;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad0;
+ goto out_uioinfo;
}
priv-uioinfo = uioinfo;
spin_lock_init(priv-lock);
- priv-flags = 0; /* interrupt is enabled to begin with */
+ /* Interrupt is enabled to begin with,
+ * that's why UIO_IRQ_DISABLED flag is not initially set.
+ */
+ priv-flags = uioinfo_alloced ? BIT_MASK(UIO_INFO_ALLOCED) : 0;
priv-pdev = pdev;
if (!uioinfo-irq) {
ret = platform_get_irq(pdev, 0);
if (ret 0) {
dev_err(pdev-dev, failed to get IRQ\n);
- goto bad0;
+ goto out_priv;
}
uioinfo-irq = ret;
}
@@ -205,19 +215,19 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
ret = uio_register_device(pdev-dev, priv-uioinfo);
if (ret) {
[GIT PULL] UIO changes for v3.8
Hi Greg, my new UIO tree is based on your current char-misc tree. Please pull the following two patches. Thanks, Hans The following changes since commit 1ebaf4f4e6912199f8a4e30ba3ab55da2b71bcdf: Merge branch 'x86-timers-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip (2012-12-11 20:01:33 -0800) are available in the git repository at: git://hansjkoch.de:/home/repos/git/char-misc.git uio-devel for you to fetch changes up to d91796fe825b7ebd8ed4150c6886faeb7c3982b1: uio: fix possible memory leak in uio.c (2012-12-13 18:55:59 +0100) Cong Ding (1): uio: fix possible memory leak in uio.c Hans J. Koch (1): uio: Fix warning: 'ret' might be used uninitialized drivers/uio/uio.c | 18 -- 1 files changed, 12 insertions(+), 6 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks
On Wed, Dec 12, 2012 at 07:08:18AM -0800, Greg KH wrote: > On Wed, Dec 12, 2012 at 09:56:16AM +0100, Benedikt Spranger wrote: > > Am Wed, 12 Dec 2012 09:50:54 +0100 > > schrieb "Hans J. Koch" : > > > > > On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote: > > > > Yes, but what does that have to do with this in-kernel, internal api? > > > > > > Ah, OK. You're right, the commit message is confusing. > > > > > > Bene, it's enough to say we drop the inode parameter because nobody > > > ever needed it. > > I am fine with that. > > > > > I cannot see why this also helps with the other problem. > > It would help, because we can defer calling the release hook until the > > last mmap user is gone. In this case the inode pointer may not be valid > > anymore. > > Which, again, is the same for any in-kernel driver with these types of > callbacks. Is that a general mmap problem that wants to be fixed? hjk > > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks
On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote:
> On Wed, Dec 12, 2012 at 02:42:22AM +0100, Hans J. Koch wrote:
> > On Tue, Dec 11, 2012 at 03:20:32PM -0800, Greg KH wrote:
> > > On Wed, Dec 12, 2012 at 12:12:02AM +0100, Benedikt Spranger wrote:
> > > > The inode parameter is unused by in kernel users of UIO.
> > >
> > > Ok.
> > >
> > > > Also the inode parameter makes it hard to resolve the existing open(),
> > > > mmap() and close() difficulty.
> > >
> > > I don't understand, what do you mean by this? What is this parameter
> > > causing problems with?
> >
> > The problem is that according to POSIX, it is guaranteed that in userspace
> > you can do
> >
> > fd = open("/dev/uio0", ...)
> > ptr = mmap(...fd...)
> > close(fd)
> >
> > with ptr still being valid and useable after that.
>
> Yes, but what does that have to do with this in-kernel, internal api?
Ah, OK. You're right, the commit message is confusing.
Bene, it's enough to say we drop the inode parameter because nobody
ever needed it. I cannot see why this also helps with the other problem.
Thanks,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks
On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote: On Wed, Dec 12, 2012 at 02:42:22AM +0100, Hans J. Koch wrote: On Tue, Dec 11, 2012 at 03:20:32PM -0800, Greg KH wrote: On Wed, Dec 12, 2012 at 12:12:02AM +0100, Benedikt Spranger wrote: The inode parameter is unused by in kernel users of UIO. Ok. Also the inode parameter makes it hard to resolve the existing open(), mmap() and close() difficulty. I don't understand, what do you mean by this? What is this parameter causing problems with? The problem is that according to POSIX, it is guaranteed that in userspace you can do fd = open(/dev/uio0, ...) ptr = mmap(...fd...) close(fd) with ptr still being valid and useable after that. Yes, but what does that have to do with this in-kernel, internal api? Ah, OK. You're right, the commit message is confusing. Bene, it's enough to say we drop the inode parameter because nobody ever needed it. I cannot see why this also helps with the other problem. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks
On Wed, Dec 12, 2012 at 07:08:18AM -0800, Greg KH wrote: On Wed, Dec 12, 2012 at 09:56:16AM +0100, Benedikt Spranger wrote: Am Wed, 12 Dec 2012 09:50:54 +0100 schrieb Hans J. Koch h...@hansjkoch.de: On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote: Yes, but what does that have to do with this in-kernel, internal api? Ah, OK. You're right, the commit message is confusing. Bene, it's enough to say we drop the inode parameter because nobody ever needed it. I am fine with that. I cannot see why this also helps with the other problem. It would help, because we can defer calling the release hook until the last mmap user is gone. In this case the inode pointer may not be valid anymore. Which, again, is the same for any in-kernel driver with these types of callbacks. Is that a general mmap problem that wants to be fixed? hjk greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] uio: add warning to documentation
On Tue, Dec 11, 2012 at 03:18:16PM -0800, Greg KH wrote: > On Wed, Dec 12, 2012 at 12:12:01AM +0100, Benedikt Spranger wrote: > > The documentation has no clear statement to the POSIX 1003.1 mmap() > > feature, wich allows open(), mmap(), close() while the mmaped pointer is > > valid. > > The release() hook inveigled driver programmer to activate owermanagement > > functuonality in the release hook. This may harm. > > > > Signed-off-by: Benedikt Spranger > > --- > > Documentation/DocBook/uio-howto.tmpl |7 ++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/DocBook/uio-howto.tmpl > > b/Documentation/DocBook/uio-howto.tmpl > > index ac3d001..59a886d 100644 > > --- a/Documentation/DocBook/uio-howto.tmpl > > +++ b/Documentation/DocBook/uio-howto.tmpl > > @@ -499,8 +499,13 @@ device is actually used. > > > > int (*release)(struct uio_info *info, struct inode *inode) > > : Optional. If you define your own > > -open(), you will probably also want a custom > > +release(), you will probably also want a custom > > release() function. > > That sentance no longer makes sense. > > > +CAVE: The release hook may be processed, even if a mmap is > > aktive. > > Huh? I think that's right. You can successfully close() a device while userspace is still using a mapping. If the driver doesn't prevent it, userspace will fail with a SIGBUS when accessing the mapping the next time. > > > +Disabling clocks or other powermanagement functionality may cause a system > > +crash, hangup or other unwanted sideeffects. > > +The mmap() function shall add an extra reference to > > the file associated with the file descriptor fildes which is not removed by > > a subsequent close() on that file descriptor. This reference shall be > > removed when there are no more mappings to the file. > > + > xlink:href="http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html;>IEEE > > Std 1003.1, 2004 Edition, mmap() > > It's not up to us to document the mmap system call here, you should know > how to use it if you write a program with it, right? In general, I agree. But in this case, I don't think that this is an mmap() feature well known to all programmers (In fact, I wasn't aware of that until Bene told me). Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks
On Tue, Dec 11, 2012 at 03:20:32PM -0800, Greg KH wrote:
> On Wed, Dec 12, 2012 at 12:12:02AM +0100, Benedikt Spranger wrote:
> > The inode parameter is unused by in kernel users of UIO.
>
> Ok.
>
> > Also the inode parameter makes it hard to resolve the existing open(),
> > mmap() and close() difficulty.
>
> I don't understand, what do you mean by this? What is this parameter
> causing problems with?
The problem is that according to POSIX, it is guaranteed that in userspace
you can do
fd = open("/dev/uio0", ...)
ptr = mmap(...fd...)
close(fd)
with ptr still being valid and useable after that.
Thanks,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks
On Tue, Dec 11, 2012 at 03:20:32PM -0800, Greg KH wrote: On Wed, Dec 12, 2012 at 12:12:02AM +0100, Benedikt Spranger wrote: The inode parameter is unused by in kernel users of UIO. Ok. Also the inode parameter makes it hard to resolve the existing open(), mmap() and close() difficulty. I don't understand, what do you mean by this? What is this parameter causing problems with? The problem is that according to POSIX, it is guaranteed that in userspace you can do fd = open(/dev/uio0, ...) ptr = mmap(...fd...) close(fd) with ptr still being valid and useable after that. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] uio: add warning to documentation
On Tue, Dec 11, 2012 at 03:18:16PM -0800, Greg KH wrote: On Wed, Dec 12, 2012 at 12:12:01AM +0100, Benedikt Spranger wrote: The documentation has no clear statement to the POSIX 1003.1 mmap() feature, wich allows open(), mmap(), close() while the mmaped pointer is valid. The release() hook inveigled driver programmer to activate owermanagement functuonality in the release hook. This may harm. Signed-off-by: Benedikt Spranger b.spran...@linutronix.de --- Documentation/DocBook/uio-howto.tmpl |7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl index ac3d001..59a886d 100644 --- a/Documentation/DocBook/uio-howto.tmpl +++ b/Documentation/DocBook/uio-howto.tmpl @@ -499,8 +499,13 @@ device is actually used. listitempara varnameint (*release)(struct uio_info *info, struct inode *inode) /varname: Optional. If you define your own -functionopen()/function, you will probably also want a custom +functionrelease()/function, you will probably also want a custom functionrelease()/function function. That sentance no longer makes sense. +/paraparaCAVE: The release hook may be processed, even if a mmap is aktive. Huh? I think that's right. You can successfully close() a device while userspace is still using a mapping. If the driver doesn't prevent it, userspace will fail with a SIGBUS when accessing the mapping the next time. +Disabling clocks or other powermanagement functionality may cause a system +crash, hangup or other unwanted sideeffects. +/paraparaemphasisThe mmap() function shall add an extra reference to the file associated with the file descriptor fildes which is not removed by a subsequent close() on that file descriptor. This reference shall be removed when there are no more mappings to the file./emphasis/parapara +link xlink:href=http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html;IEEE Std 1003.1, 2004 Edition, mmap()/link It's not up to us to document the mmap system call here, you should know how to use it if you write a program with it, right? In general, I agree. But in this case, I don't think that this is an mmap() feature well known to all programmers (In fact, I wasn't aware of that until Bene told me). Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/1] uio.c: solve memory leak
On Thu, Nov 29, 2012 at 05:40:00PM +, Cong Ding wrote:
> In version 1, I forgot to modify the same bug in the first loop.
>
> we have to call kobject_put() to clean up the kobject after function
> kobject_init(), kobject_add(), or kobject_uevent() is called.
>
> Signed-off-by: Cong Ding
Signed-off-by: "Hans J. Koch"
> ---
> drivers/uio/uio.c | 16 ++--
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..79774d3 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -291,10 +291,10 @@ static int uio_dev_add_attributes(struct uio_device
> *idev)
> mem->map = map;
> ret = kobject_add(>kobj, idev->map_dir, "map%d", mi);
> if (ret)
> - goto err_map;
> + goto err_map_kobj;
> ret = kobject_uevent(>kobj, KOBJ_ADD);
> if (ret)
> - goto err_map;
> + goto err_map_kobj;
> }
>
> for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
> @@ -317,23 +317,27 @@ static int uio_dev_add_attributes(struct uio_device
> *idev)
> ret = kobject_add(>kobj, idev->portio_dir,
> "port%d", pi);
> if (ret)
> - goto err_portio;
> + goto err_portio_kobj;
> ret = kobject_uevent(>kobj, KOBJ_ADD);
> if (ret)
> - goto err_portio;
> + goto err_portio_kobj;
> }
>
> return 0;
>
> err_portio:
> - for (pi--; pi >= 0; pi--) {
> + pi--;
> +err_portio_kobj:
> + for (; pi >= 0; pi--) {
> port = >info->port[pi];
> portio = port->portio;
> kobject_put(>kobj);
> }
> kobject_put(idev->portio_dir);
> err_map:
> - for (mi--; mi>=0; mi--) {
> + mi--;
> +err_map_kobj:
> + for (; mi >= 0; mi--) {
> mem = >info->mem[mi];
> map = mem->map;
> kobject_put(>kobj);
> --
> 1.7.4.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Mon, Dec 10, 2012 at 12:24:04PM +0200, Vitalii Demianets wrote: > > > > Hi Vitalii, > > > > thanks a lot for analyzing the problem so thoroughly. It made me review > > > > uio_pdrv_genirq.c again, and I noticed several issues and came to the > > > > following conclusions: > > > > > > > > 1.) priv->lock is completely unnecessary. It is only used in one > > > >function, > > > > so there's nothing it could possibly protect. > > > > > > > > 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also > > > > unnecessary. We can simply use enable_irq and disable_irq in both the > > > > irq handler and in uio_pdrv_genirq_irqcontrol. > > > > > > > > We should go "back to the roots" and have a look at how UIO works. > > > > The workflow it is intended for is like this: > > > > > > > > 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware > > > > has its interrupt disabled at that time. > > > > > > > > 2.) uio_pdrv_genirq is loaded. Kernel enables the irq. > > > > > > > > 3.) Userspace part of the driver comes up. It will initialize the > > > > hardware > > > > (including setting the bits that enable the interrupt). > > > > > > > > 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically, > > > > there'll be a loop or a thread with this blocking read() at the > > > > beginning > > > > (usually using the select() call). > > > > > > > > 5.) At some time, a hardware interrupt will occur. The irq handler in > > > > kernel space will be called, only to disable the irq. This will also > > > > cause the UIO core to make /dev/uioX readable. > > > > > > > > 6.) Userspace's blocking read returns. Userspace does its work by > > > > reading/writing device memory. > > > > > > > > 7.) As the last thing, Userspace writes a "1" to /dev/uioX, which causes > > > > uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt. > > > > > > > > 8.) Goto 4.) > > > > > > > > We should also remember that uio_pdrv_genirq_handler() is NOT a real > > > > hardware irq handler. The real handler is in the UIO core, which will > > > > increment the event number and wake up userspace. > > > > > > > > So, although your scenario clearly shows a subtle race condition, there > > > > is none. If userspace does stupid things, no harm will be done to the > > > > kernel. > > > > > > With all due respect, I disagree here. If userspace does stupid things, > > > unintentionally because of poorly written code or intentionally because > > > of > a > > > malicious purpose, we could have irq depth counter disbalanced which, in > > > turn, could lead to the interrupt not enabled when it should be. > > > > It's just the interrupt of a broken UIO userspace driver. Note that this is > > also a security mechanism, disabling the irq when it is not handled > > properly. > > > > > In which > > > case the whole system would stop doing useful things which it was > > > designed > > > to do. > > > > If the kernel irq depth counter can't track the irq, I don't really see a > > point in fixing it by adding extra flags in a simple driver. > > > > The flag in driver would be needed if we supposed that userspace can do > stupid > things. For example, userspace can call write() twice with zero data. Which > would lead to disabling irq twice. Which means that someone would need to > enable irq twice. Which is not in the userspace-kernel interaction scenario > you so clearly described above previously. Indeed. It means that somebody wrote bad userspace code, and it needs to be fixed there. > > Also, without that flag the irq could be disabled twice even when userspace > do > not do double-disable. It could happen when userspace is writing to the > device file AND irq is running at this same moment on another CPU core > concurrently. If that happens, then either userspace is broken or the device cannot be handled by a UIO driver (e.g. because irq rate is too high which is usually an indicator for bad hardware design). > Yes, you are right that such a situation could be avoided if userspace code > was written carefully. But how can you assure that every user will write good > code? I don't care at all. I know the quality of code written in industry too well. They'll write bad code. All we can do is prevent damage to others. > > > > > > > > If userspace is designed the way described above (and in the > > > > documentation), it will always wake up with its interrupt disabled, do > > > > its work, and then re-enable the interrupt. You can probably think of > > > > a few things userspace could do to screw things up. But that's not our > > > > problem. > > > > > > > > > > Disagree again. I think we can not leave a hole in kernel which opens DOS > > > possibilities and hope userspace will behave well. There are always > > > errors > > > in the code. And security issues, too. > > > > UIO is a risky thing to begin with. There are many more possibilities to > > do bad things. That's why UIO
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Mon, Dec 10, 2012 at 11:03:59AM +0200, Vitalii Demianets wrote: > On Saturday 08 December 2012 01:47:21 Hans J. Koch wrote: > > On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote: > > > > On second thought, we can't call enable_irq()/disable_irq() > > > > unconditionally because of the potential disable counter > > > > (irq_desc->depth) disbalance. That's why we need UIO_IRQ_DISABLED flag, > > > > and that's why we should check it in uio_pdrv_genirq_irqcontrol(). > > > > On the other hand, you are right in that we don't need to check it > > > > inside irq handler. Inside irq handler we can disable irq and set the > > > > flag unconditionally, because: > > > > a) We know for sure that irqs are enabled, because we are inside > > > > (not-shared) irq handler; > > > > and > > > > b) We are guarded from potential race conditions by spin_lock_irqsave() > > > > call in uio_pdrv_genirq_irqcontrol(). > > > > > > > > So,yes, we can get rid of costly atomic call to > > > > test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still > > > > don't like the idea of mixing this optimization with bug fixes in a > > > > single patch. > > > > > > On the third thought, we can't ;) > > > Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being > > > executed on CPU0 and irq handler is running concurrently on CPU1. To > > > protect from disable_irq counter disbalance we must first check current > > > irq status, and in atomic manner. Thus we prevent double-disable, one > > > from > > > uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler > > > running on CPU1. > > > Above consideration justifies current code. > > > > > > But it seems we have potential concurrency problem here anyway. Here is > > > theoretical scenario: > > > 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts > > > being executed on CPU0 with irq_on=1 and at the same time, concurrently, > > > irq handler starts being executed on CPU1; > > > 2) irq handler executes line > > >if (!test_and_set_bit(UIO_IRQ_DISABLED, >flags)) > > > as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set. > > > 3) uio_pdrv_genirq_irqcontrol() executes line > > >if (test_and_clear_bit(UIO_IRQ_DISABLED, >flags)) > > > as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now > > > UIO_IRQ_DISABLED is cleared. > > > 4) CPU0 executes enable_irq() . IRQ is enabled. "Unbalanced enable for > > > IRQ %d\n" warning is issued. > > > 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled. > > > > > > And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED > > > is cleared. Bad. > > > > > > The above scenario is purely theoretical, I have no means to check it in > > > hardware. > > > The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual > > > irq disabling inside irq handler by priv->lock. > > > > > > What do you think about it? Is it worth worrying about? > > > > Hi Vitalii, > > thanks a lot for analyzing the problem so thoroughly. It made me review > > uio_pdrv_genirq.c again, and I noticed several issues and came to the > > following conclusions: > > > > 1.) priv->lock is completely unnecessary. It is only used in one function, > > so there's nothing it could possibly protect. > > > > 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also > > unnecessary. We can simply use enable_irq and disable_irq in both the > > irq handler and in uio_pdrv_genirq_irqcontrol. > > > > We should go "back to the roots" and have a look at how UIO works. > > The workflow it is intended for is like this: > > > > 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware > > has its interrupt disabled at that time. > > > > 2.) uio_pdrv_genirq is loaded. Kernel enables the irq. > > > > 3.) Userspace part of the driver comes up. It will initialize the hardware > > (including setting the bits that enable the interrupt). > > > > 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically, > > there'll be a loop or a thread with this blocking read() at the beginning > > (usually using the select() call). > > > > 5.) At some time, a hardware interrupt will occur
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Mon, Dec 10, 2012 at 11:03:59AM +0200, Vitalii Demianets wrote: On Saturday 08 December 2012 01:47:21 Hans J. Koch wrote: On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote: On second thought, we can't call enable_irq()/disable_irq() unconditionally because of the potential disable counter (irq_desc-depth) disbalance. That's why we need UIO_IRQ_DISABLED flag, and that's why we should check it in uio_pdrv_genirq_irqcontrol(). On the other hand, you are right in that we don't need to check it inside irq handler. Inside irq handler we can disable irq and set the flag unconditionally, because: a) We know for sure that irqs are enabled, because we are inside (not-shared) irq handler; and b) We are guarded from potential race conditions by spin_lock_irqsave() call in uio_pdrv_genirq_irqcontrol(). So,yes, we can get rid of costly atomic call to test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still don't like the idea of mixing this optimization with bug fixes in a single patch. On the third thought, we can't ;) Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being executed on CPU0 and irq handler is running concurrently on CPU1. To protect from disable_irq counter disbalance we must first check current irq status, and in atomic manner. Thus we prevent double-disable, one from uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler running on CPU1. Above consideration justifies current code. But it seems we have potential concurrency problem here anyway. Here is theoretical scenario: 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts being executed on CPU0 with irq_on=1 and at the same time, concurrently, irq handler starts being executed on CPU1; 2) irq handler executes line if (!test_and_set_bit(UIO_IRQ_DISABLED, priv-flags)) as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set. 3) uio_pdrv_genirq_irqcontrol() executes line if (test_and_clear_bit(UIO_IRQ_DISABLED, priv-flags)) as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now UIO_IRQ_DISABLED is cleared. 4) CPU0 executes enable_irq() . IRQ is enabled. Unbalanced enable for IRQ %d\n warning is issued. 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled. And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED is cleared. Bad. The above scenario is purely theoretical, I have no means to check it in hardware. The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual irq disabling inside irq handler by priv-lock. What do you think about it? Is it worth worrying about? Hi Vitalii, thanks a lot for analyzing the problem so thoroughly. It made me review uio_pdrv_genirq.c again, and I noticed several issues and came to the following conclusions: 1.) priv-lock is completely unnecessary. It is only used in one function, so there's nothing it could possibly protect. 2.) All these test_and_clear_bit and test_and_set_bit calls are also unnecessary. We can simply use enable_irq and disable_irq in both the irq handler and in uio_pdrv_genirq_irqcontrol. We should go back to the roots and have a look at how UIO works. The workflow it is intended for is like this: 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware has its interrupt disabled at that time. 2.) uio_pdrv_genirq is loaded. Kernel enables the irq. 3.) Userspace part of the driver comes up. It will initialize the hardware (including setting the bits that enable the interrupt). 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically, there'll be a loop or a thread with this blocking read() at the beginning (usually using the select() call). 5.) At some time, a hardware interrupt will occur. The irq handler in kernel space will be called, only to disable the irq. This will also cause the UIO core to make /dev/uioX readable. 6.) Userspace's blocking read returns. Userspace does its work by reading/writing device memory. 7.) As the last thing, Userspace writes a 1 to /dev/uioX, which causes uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt. 8.) Goto 4.) We should also remember that uio_pdrv_genirq_handler() is NOT a real hardware irq handler. The real handler is in the UIO core, which will increment the event number and wake up userspace. So, although your scenario clearly shows a subtle race condition, there is none. If userspace does stupid things, no harm will be done to the kernel. With all due respect, I disagree here. If userspace does stupid things, unintentionally because of poorly written code or intentionally because of a malicious purpose, we could have irq depth counter disbalanced which, in turn, could lead
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Mon, Dec 10, 2012 at 12:24:04PM +0200, Vitalii Demianets wrote: Hi Vitalii, thanks a lot for analyzing the problem so thoroughly. It made me review uio_pdrv_genirq.c again, and I noticed several issues and came to the following conclusions: 1.) priv-lock is completely unnecessary. It is only used in one function, so there's nothing it could possibly protect. 2.) All these test_and_clear_bit and test_and_set_bit calls are also unnecessary. We can simply use enable_irq and disable_irq in both the irq handler and in uio_pdrv_genirq_irqcontrol. We should go back to the roots and have a look at how UIO works. The workflow it is intended for is like this: 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware has its interrupt disabled at that time. 2.) uio_pdrv_genirq is loaded. Kernel enables the irq. 3.) Userspace part of the driver comes up. It will initialize the hardware (including setting the bits that enable the interrupt). 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically, there'll be a loop or a thread with this blocking read() at the beginning (usually using the select() call). 5.) At some time, a hardware interrupt will occur. The irq handler in kernel space will be called, only to disable the irq. This will also cause the UIO core to make /dev/uioX readable. 6.) Userspace's blocking read returns. Userspace does its work by reading/writing device memory. 7.) As the last thing, Userspace writes a 1 to /dev/uioX, which causes uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt. 8.) Goto 4.) We should also remember that uio_pdrv_genirq_handler() is NOT a real hardware irq handler. The real handler is in the UIO core, which will increment the event number and wake up userspace. So, although your scenario clearly shows a subtle race condition, there is none. If userspace does stupid things, no harm will be done to the kernel. With all due respect, I disagree here. If userspace does stupid things, unintentionally because of poorly written code or intentionally because of a malicious purpose, we could have irq depth counter disbalanced which, in turn, could lead to the interrupt not enabled when it should be. It's just the interrupt of a broken UIO userspace driver. Note that this is also a security mechanism, disabling the irq when it is not handled properly. In which case the whole system would stop doing useful things which it was designed to do. If the kernel irq depth counter can't track the irq, I don't really see a point in fixing it by adding extra flags in a simple driver. The flag in driver would be needed if we supposed that userspace can do stupid things. For example, userspace can call write() twice with zero data. Which would lead to disabling irq twice. Which means that someone would need to enable irq twice. Which is not in the userspace-kernel interaction scenario you so clearly described above previously. Indeed. It means that somebody wrote bad userspace code, and it needs to be fixed there. Also, without that flag the irq could be disabled twice even when userspace do not do double-disable. It could happen when userspace is writing to the device file AND irq is running at this same moment on another CPU core concurrently. If that happens, then either userspace is broken or the device cannot be handled by a UIO driver (e.g. because irq rate is too high which is usually an indicator for bad hardware design). Yes, you are right that such a situation could be avoided if userspace code was written carefully. But how can you assure that every user will write good code? I don't care at all. I know the quality of code written in industry too well. They'll write bad code. All we can do is prevent damage to others. If userspace is designed the way described above (and in the documentation), it will always wake up with its interrupt disabled, do its work, and then re-enable the interrupt. You can probably think of a few things userspace could do to screw things up. But that's not our problem. Disagree again. I think we can not leave a hole in kernel which opens DOS possibilities and hope userspace will behave well. There are always errors in the code. And security issues, too. UIO is a risky thing to begin with. There are many more possibilities to do bad things. That's why UIO devices are root-only. We can assume that code running under root does not do bad things on purpose. If that was true, there would be no security problems in the world. Making UIO devices root-only is only a prerequisite for constructing a safe system. BTW, there are more safety-critical devices in a typical
Re: [PATCH v2 1/1] uio.c: solve memory leak
On Thu, Nov 29, 2012 at 05:40:00PM +, Cong Ding wrote:
In version 1, I forgot to modify the same bug in the first loop.
we have to call kobject_put() to clean up the kobject after function
kobject_init(), kobject_add(), or kobject_uevent() is called.
Signed-off-by: Cong Ding ding...@gmail.com
Signed-off-by: Hans J. Koch h...@hansjkoch.de
---
drivers/uio/uio.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..79774d3 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -291,10 +291,10 @@ static int uio_dev_add_attributes(struct uio_device
*idev)
mem-map = map;
ret = kobject_add(map-kobj, idev-map_dir, map%d, mi);
if (ret)
- goto err_map;
+ goto err_map_kobj;
ret = kobject_uevent(map-kobj, KOBJ_ADD);
if (ret)
- goto err_map;
+ goto err_map_kobj;
}
for (pi = 0; pi MAX_UIO_PORT_REGIONS; pi++) {
@@ -317,23 +317,27 @@ static int uio_dev_add_attributes(struct uio_device
*idev)
ret = kobject_add(portio-kobj, idev-portio_dir,
port%d, pi);
if (ret)
- goto err_portio;
+ goto err_portio_kobj;
ret = kobject_uevent(portio-kobj, KOBJ_ADD);
if (ret)
- goto err_portio;
+ goto err_portio_kobj;
}
return 0;
err_portio:
- for (pi--; pi = 0; pi--) {
+ pi--;
+err_portio_kobj:
+ for (; pi = 0; pi--) {
port = idev-info-port[pi];
portio = port-portio;
kobject_put(portio-kobj);
}
kobject_put(idev-portio_dir);
err_map:
- for (mi--; mi=0; mi--) {
+ mi--;
+err_map_kobj:
+ for (; mi = 0; mi--) {
mem = idev-info-mem[mi];
map = mem-map;
kobject_put(map-kobj);
--
1.7.4.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/1] uio.c: solve memory leak
On Fri, Dec 07, 2012 at 12:02:11AM +0100, Cong Ding wrote: > ping Hans, did you have any comment on this? Sounds right what you say. Is your patch v2 your final solution, or would you like to come up with v3? Thanks a lot for your patience and your thorough analysis. Hans > > - cong > > On Fri, Nov 30, 2012 at 12:03 PM, Cong Ding wrote: > > Hi Hans, I think the memory allocated with kzalloc is properly freed > > by calling kobject_put. > > > > I can give a simple explanation. > > > > 1) when we call kobject_init, the parameter portio_attr_type is > > passed in. portio_attr_type includes a function pointer to > > portio_release, which releases the memory of portio. > > > > 2) when we call kobject_put, kref_put is called with the pointer of > > function kobject_release. > > 3) kref_put calls kref_sub, with the same pointer of function > > kobject_release. > > 4) and kref_put calls the function kboject_release if > > atomic_sub_and_test returns true > > > > 5) let's look at what kobject_release is. it calls kobject_cleanup, > > and kobject_cleanup calls t->release(kobj) where t->release is exactly > > the function we passed in through portio_init at step (1). so function > > portio_release is called, and the memory allocated with kzalloc is > > freed. > > > > If there are anything wrong in my analysis, please feel free to let me know. > > > > Personally, I suggest to add a function to create and release > > uio_portio, which is similar as kobject_create and kobject_put in file > > lib/kobject.c. In this way, it avoid other readers thinking the memory > > is not freed (and we should add some comments here). For example, > > uio_portio_create call kzalloc and kboject_init, and returns > > uio_portio, which is similar as function kobject_create; and > > uio_portio_release calls kobject_put to release the memory. And we do > > same thing for uio_map. > > > > The usage here is quite strange, but it works. If I write this > > function from zero, I will use a pointer to kobject in uio_portio > > struct instead of kobject struct itself. In this case I can call > > kobject_create instead of kobject_init, and then we do both > > kzalloc(uio_portio) and kfree(uio_portio) in the file uio.c. > > > > Best, > > Cong > > > > On Fri, Nov 30, 2012 at 1:13 AM, Hans J. Koch wrote: > >> There's still another bug: The memory allocated with kzalloc is > >> never freed. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote: > > > > On second thought, we can't call enable_irq()/disable_irq() unconditionally > > because of the potential disable counter (irq_desc->depth) disbalance. > > That's why we need UIO_IRQ_DISABLED flag, and that's why we should check it > > in uio_pdrv_genirq_irqcontrol(). > > On the other hand, you are right in that we don't need to check it inside > > irq handler. Inside irq handler we can disable irq and set the flag > > unconditionally, because: > > a) We know for sure that irqs are enabled, because we are inside > > (not-shared) irq handler; > > and > > b) We are guarded from potential race conditions by spin_lock_irqsave() > > call in uio_pdrv_genirq_irqcontrol(). > > > > So,yes, we can get rid of costly atomic call to > > test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still don't > > like the idea of mixing this optimization with bug fixes in a single patch. > > On the third thought, we can't ;) > Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being executed > on > CPU0 and irq handler is running concurrently on CPU1. To protect from > disable_irq counter disbalance we must first check current irq status, and in > atomic manner. Thus we prevent double-disable, one from > uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler > running on CPU1. > Above consideration justifies current code. > > But it seems we have potential concurrency problem here anyway. Here is > theoretical scenario: > 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts being > executed on CPU0 with irq_on=1 and at the same time, concurrently, irq > handler starts being executed on CPU1; > 2) irq handler executes line >if (!test_and_set_bit(UIO_IRQ_DISABLED, >flags)) > as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set. > 3) uio_pdrv_genirq_irqcontrol() executes line >if (test_and_clear_bit(UIO_IRQ_DISABLED, >flags)) > as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now > UIO_IRQ_DISABLED is cleared. > 4) CPU0 executes enable_irq() . IRQ is enabled. "Unbalanced enable for > IRQ %d\n" warning is issued. > 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled. > > And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED is > cleared. Bad. > > The above scenario is purely theoretical, I have no means to check it in > hardware. > The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual irq > disabling inside irq handler by priv->lock. > > What do you think about it? Is it worth worrying about? Hi Vitalii, thanks a lot for analyzing the problem so thoroughly. It made me review uio_pdrv_genirq.c again, and I noticed several issues and came to the following conclusions: 1.) priv->lock is completely unnecessary. It is only used in one function, so there's nothing it could possibly protect. 2.) All these "test_and_clear_bit" and "test_and_set_bit" calls are also unnecessary. We can simply use enable_irq and disable_irq in both the irq handler and in uio_pdrv_genirq_irqcontrol. We should go "back to the roots" and have a look at how UIO works. The workflow it is intended for is like this: 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware has its interrupt disabled at that time. 2.) uio_pdrv_genirq is loaded. Kernel enables the irq. 3.) Userspace part of the driver comes up. It will initialize the hardware (including setting the bits that enable the interrupt). 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically, there'll be a loop or a thread with this blocking read() at the beginning (usually using the select() call). 5.) At some time, a hardware interrupt will occur. The irq handler in kernel space will be called, only to disable the irq. This will also cause the UIO core to make /dev/uioX readable. 6.) Userspace's blocking read returns. Userspace does its work by reading/writing device memory. 7.) As the last thing, Userspace writes a "1" to /dev/uioX, which causes uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt. 8.) Goto 4.) We should also remember that uio_pdrv_genirq_handler() is NOT a real hardware irq handler. The real handler is in the UIO core, which will increment the event number and wake up userspace. So, although your scenario clearly shows a subtle race condition, there is none. If userspace does stupid things, no harm will be done to the kernel. If userspace is designed the way described above (and in the documentation), it will always wake up with its interrupt disabled, do its work, and then re-enable the interrupt. You can probably think of a few things userspace could do to screw things up. But that's not our problem. Could you hack up a patch for that? I think it should start with removing uio_pdrv_genirq_platdata->lock and uio_pdrv_genirq_platdata->flags... Thanks again for your work. What do you
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Fri, Dec 07, 2012 at 05:00:54PM +0200, Vitalii Demianets wrote: On second thought, we can't call enable_irq()/disable_irq() unconditionally because of the potential disable counter (irq_desc-depth) disbalance. That's why we need UIO_IRQ_DISABLED flag, and that's why we should check it in uio_pdrv_genirq_irqcontrol(). On the other hand, you are right in that we don't need to check it inside irq handler. Inside irq handler we can disable irq and set the flag unconditionally, because: a) We know for sure that irqs are enabled, because we are inside (not-shared) irq handler; and b) We are guarded from potential race conditions by spin_lock_irqsave() call in uio_pdrv_genirq_irqcontrol(). So,yes, we can get rid of costly atomic call to test_and_set_bit(UIO_IRQ_DISABLED,..) inside irq handler. But I still don't like the idea of mixing this optimization with bug fixes in a single patch. On the third thought, we can't ;) Imagine the SMP system where uio_pdrv_genirq_irqcontrol() is being executed on CPU0 and irq handler is running concurrently on CPU1. To protect from disable_irq counter disbalance we must first check current irq status, and in atomic manner. Thus we prevent double-disable, one from uio_pdrv_genirq_irqcontrol() running on CPU0 and another form irq handler running on CPU1. Above consideration justifies current code. But it seems we have potential concurrency problem here anyway. Here is theoretical scenario: 1) Initial state: irq is enabled, uio_pdrv_genirq_irqcontrol() starts being executed on CPU0 with irq_on=1 and at the same time, concurrently, irq handler starts being executed on CPU1; 2) irq handler executes line if (!test_and_set_bit(UIO_IRQ_DISABLED, priv-flags)) as irq was enabled, the condition holds. And now UIO_IRQ_DISABLED is set. 3) uio_pdrv_genirq_irqcontrol() executes line if (test_and_clear_bit(UIO_IRQ_DISABLED, priv-flags)) as UIO_IRQ_DISABLED was set by CPU1, the condition holds. And now UIO_IRQ_DISABLED is cleared. 4) CPU0 executes enable_irq() . IRQ is enabled. Unbalanced enable for IRQ %d\n warning is issued. 5) irq handler on CPU1 executes disable_irq_nosync(). IRQ is disabled. And now we are in situation where IRQ is disabled, but UIO_IRQ_DISABLED is cleared. Bad. The above scenario is purely theoretical, I have no means to check it in hardware. The (theoretical) solution is to guard UIO_IRQ_DISABLED test and actual irq disabling inside irq handler by priv-lock. What do you think about it? Is it worth worrying about? Hi Vitalii, thanks a lot for analyzing the problem so thoroughly. It made me review uio_pdrv_genirq.c again, and I noticed several issues and came to the following conclusions: 1.) priv-lock is completely unnecessary. It is only used in one function, so there's nothing it could possibly protect. 2.) All these test_and_clear_bit and test_and_set_bit calls are also unnecessary. We can simply use enable_irq and disable_irq in both the irq handler and in uio_pdrv_genirq_irqcontrol. We should go back to the roots and have a look at how UIO works. The workflow it is intended for is like this: 1.) Hardware is in Reset State (e.g. after boot). Any decent hardware has its interrupt disabled at that time. 2.) uio_pdrv_genirq is loaded. Kernel enables the irq. 3.) Userspace part of the driver comes up. It will initialize the hardware (including setting the bits that enable the interrupt). 4.) Userspace will then issue a blocking read() on /dev/uioX. Typically, there'll be a loop or a thread with this blocking read() at the beginning (usually using the select() call). 5.) At some time, a hardware interrupt will occur. The irq handler in kernel space will be called, only to disable the irq. This will also cause the UIO core to make /dev/uioX readable. 6.) Userspace's blocking read returns. Userspace does its work by reading/writing device memory. 7.) As the last thing, Userspace writes a 1 to /dev/uioX, which causes uio_pdrv_genirq_irqcontrol to be called, re-enabling the interrupt. 8.) Goto 4.) We should also remember that uio_pdrv_genirq_handler() is NOT a real hardware irq handler. The real handler is in the UIO core, which will increment the event number and wake up userspace. So, although your scenario clearly shows a subtle race condition, there is none. If userspace does stupid things, no harm will be done to the kernel. If userspace is designed the way described above (and in the documentation), it will always wake up with its interrupt disabled, do its work, and then re-enable the interrupt. You can probably think of a few things userspace could do to screw things up. But that's not our problem. Could you hack up a patch for that? I think it should start with removing uio_pdrv_genirq_platdata-lock and uio_pdrv_genirq_platdata-flags... Thanks again for your work. What do you think about my theory? Thanks, Hans -- To unsubscribe from this list: send the
Re: [PATCH v2 1/1] uio.c: solve memory leak
On Fri, Dec 07, 2012 at 12:02:11AM +0100, Cong Ding wrote: ping Hans, did you have any comment on this? Sounds right what you say. Is your patch v2 your final solution, or would you like to come up with v3? Thanks a lot for your patience and your thorough analysis. Hans - cong On Fri, Nov 30, 2012 at 12:03 PM, Cong Ding ding...@gmail.com wrote: Hi Hans, I think the memory allocated with kzalloc is properly freed by calling kobject_put. I can give a simple explanation. 1) when we call kobject_init, the parameter portio_attr_type is passed in. portio_attr_type includes a function pointer to portio_release, which releases the memory of portio. 2) when we call kobject_put, kref_put is called with the pointer of function kobject_release. 3) kref_put calls kref_sub, with the same pointer of function kobject_release. 4) and kref_put calls the function kboject_release if atomic_sub_and_test returns true 5) let's look at what kobject_release is. it calls kobject_cleanup, and kobject_cleanup calls t-release(kobj) where t-release is exactly the function we passed in through portio_init at step (1). so function portio_release is called, and the memory allocated with kzalloc is freed. If there are anything wrong in my analysis, please feel free to let me know. Personally, I suggest to add a function to create and release uio_portio, which is similar as kobject_create and kobject_put in file lib/kobject.c. In this way, it avoid other readers thinking the memory is not freed (and we should add some comments here). For example, uio_portio_create call kzalloc and kboject_init, and returns uio_portio, which is similar as function kobject_create; and uio_portio_release calls kobject_put to release the memory. And we do same thing for uio_map. The usage here is quite strange, but it works. If I write this function from zero, I will use a pointer to kobject in uio_portio struct instead of kobject struct itself. In this case I can call kobject_create instead of kobject_init, and then we do both kzalloc(uio_portio) and kfree(uio_portio) in the file uio.c. Best, Cong On Fri, Nov 30, 2012 at 1:13 AM, Hans J. Koch h...@hansjkoch.de wrote: There's still another bug: The memory allocated with kzalloc is never freed. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] uio: be aware of open(), mmap(), close()
On Thu, Dec 06, 2012 at 01:44:56PM +0100, Benedikt Spranger wrote:
> After open(), mmap(), close() the mmaped pointer is valid. Removing the
> underlaying module causes a Oops or other strange effects. Keep all structures
> valid till the last user is gone.
>
> Signed-off-by: Benedikt Spranger
> ---
> drivers/uio/uio.c | 59
> ++---
> drivers/uio/uio_pdrv_genirq.c |4 +--
> include/linux/uio_driver.h|4 +--
> 3 files changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..b96499a 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -433,8 +433,22 @@ static irqreturn_t uio_interrupt(int irq, void *dev_id)
> struct uio_listener {
> struct uio_device *dev;
> s32 event_count;
> + struct kref kref;
> };
>
> +static void uio_release_listener(struct kref *kref)
> +{
> + struct uio_listener *listener = container_of(kref, struct uio_listener,
> + kref);
> + struct uio_device *idev = listener->dev;
> +
> + if (idev->info->release)
> + idev->info->release(idev->info);
> +
> + module_put(idev->owner);
> + kfree(listener);
> +}
> +
> static int uio_open(struct inode *inode, struct file *filep)
> {
> struct uio_device *idev;
> @@ -465,10 +479,13 @@ static int uio_open(struct inode *inode, struct file
> *filep)
> filep->private_data = listener;
>
> if (idev->info->open) {
> - ret = idev->info->open(idev->info, inode);
> + ret = idev->info->open(idev->info);
> if (ret)
> goto err_infoopen;
> }
> +
> + kref_init(>kref);
> +
> return 0;
>
> err_infoopen:
> @@ -491,16 +508,11 @@ static int uio_fasync(int fd, struct file *filep, int
> on)
>
> static int uio_release(struct inode *inode, struct file *filep)
> {
> - int ret = 0;
> struct uio_listener *listener = filep->private_data;
> - struct uio_device *idev = listener->dev;
>
> - if (idev->info->release)
> - ret = idev->info->release(idev->info, inode);
> + kref_put(>kref, uio_release_listener);
>
> - module_put(idev->owner);
> - kfree(listener);
> - return ret;
> + return 0;
> }
>
> static unsigned int uio_poll(struct file *filep, poll_table *wait)
> @@ -605,19 +617,22 @@ static int uio_find_mem_index(struct vm_area_struct
> *vma)
>
> static void uio_vma_open(struct vm_area_struct *vma)
> {
> - struct uio_device *idev = vma->vm_private_data;
> - idev->vma_count++;
> + struct uio_listener *listener = vma->vm_private_data;
> +
> + kref_get(>kref);
> }
>
> static void uio_vma_close(struct vm_area_struct *vma)
> {
> - struct uio_device *idev = vma->vm_private_data;
> - idev->vma_count--;
> + struct uio_listener *listener = vma->vm_private_data;
> +
> + kref_put(>kref, uio_release_listener);
> }
>
> static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> {
> - struct uio_device *idev = vma->vm_private_data;
> + struct uio_listener *listener = vma->vm_private_data;
> + struct uio_device *idev = listener->dev;
> struct page *page;
> unsigned long offset;
>
> @@ -646,20 +661,34 @@ static const struct vm_operations_struct uio_vm_ops = {
> .fault = uio_vma_fault,
> };
>
> +static const struct vm_operations_struct uio_phys_ops = {
> + .open = uio_vma_open,
> + .close = uio_vma_close,
> +};
> +
> static int uio_mmap_physical(struct vm_area_struct *vma)
> {
> struct uio_device *idev = vma->vm_private_data;
> int mi = uio_find_mem_index(vma);
> + int ret;
> +
> if (mi < 0)
> return -EINVAL;
>
> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + vma->vm_ops = _phys_ops;
>
> - return remap_pfn_range(vma,
> + ret = remap_pfn_range(vma,
> vma->vm_start,
> idev->info->mem[mi].addr >> PAGE_SHIFT,
> vma->vm_end - vma->vm_start,
> vma->vm_page_prot);
> + if (ret)
> + return ret;
> +
> + uio_vma_open(vma);
> +
> + return 0;
> }
>
> static int uio_mmap_logical(struct vm_area_struct *vma)
> @@ -681,7 +710,7 @@ static int uio_mmap(struct file *filep, struct
> vm_area_struct *vma)
> if (vma->vm_end < vma->vm_start)
> return -EINVAL;
>
> - vma->vm_private_data = idev;
> + vma->vm_private_data = listener;
>
> mi = uio_find_mem_index(vma);
> if (mi < 0)
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 42202cd..a4e32fa 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -37,7 +37,7 @@ struct uio_pdrv_genirq_platdata {
> struct platform_device *pdev;
> };
>
> -static
Re: [PATCH 2/2] uio: avoid module unloding of in module created UIO devices
On Thu, Dec 06, 2012 at 01:44:57PM +0100, Benedikt Spranger wrote:
> A kernel module can create a uio device. Get a reference to the module, if the
> UIO device is in use. Otherwise the device can be removed and a uio write or
> an
> access to am mmaped memory can cause a kernel Oops or other strange effects.
>
> Signed-off-by: Benedikt Spranger
> ---
> drivers/uio/uio.c |8
> include/linux/uio_driver.h |1 +
> 2 files changed, 9 insertions(+)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index b96499a..3b62da0 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -446,6 +446,7 @@ static void uio_release_listener(struct kref *kref)
> idev->info->release(idev->info);
>
> module_put(idev->owner);
> + module_put(idev->info->owner);
> kfree(listener);
> }
>
> @@ -468,6 +469,11 @@ static int uio_open(struct inode *inode, struct file
> *filep)
> goto out;
> }
>
> + if (!try_module_get(idev->info->owner)) {
> + ret = -ENODEV;
> + goto err_get_module;
> + }
> +
> listener = kmalloc(sizeof(*listener), GFP_KERNEL);
> if (!listener) {
> ret = -ENOMEM;
> @@ -493,6 +499,8 @@ err_infoopen:
>
> err_alloc_listener:
> module_put(idev->owner);
> +err_get_module:
> + module_put(idev->owner);
>
> out:
> return ret;
> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
> index 1bc6493..2862de23 100644
> --- a/include/linux/uio_driver.h
> +++ b/include/linux/uio_driver.h
> @@ -95,6 +95,7 @@ struct uio_info {
> int (*open)(struct uio_info *info);
> void (*release)(struct uio_info *info);
> int (*irqcontrol)(struct uio_info *info, s32 irq_on);
> + struct module *owner;
Where is that needed? I won't add a new element to struct uio_info without
an in-kernel user.
Thanks,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Thu, Dec 06, 2012 at 11:11:38AM +0200, Vitalii Demianets wrote: > On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote: > > > @@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, > struct uio_info *dev_info) > > >* remember the state so we can allow user space to enable it later. > > >*/ > > > > > > - if (!test_and_set_bit(0, >flags)) > > > + if (!test_and_set_bit(UIO_IRQ_DISABLED, >flags)) > > > disable_irq_nosync(irq); > > > > That is not safe. That flag can only be changed by userspace, and if the > > userspace part is not running, the CPU on which the irq is pending will > > hang. > > The handler has to disable the interrupt in any case. > > (The original version had the same problem, I know...) > > > > Perhaps I'm missing something here, but I don't see your point. If userspace > is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by writing to the > device file, then on the first invocation of interrupt handler that IRQ is > disabled by disable_irq_nosync() and that's the end of story - no more irqs, > no problems. Until userspace writes non-zero to the device file, of course. If you run into the irq handler, the interrupt was obviously enabled. Since irq sharing is not supported by this driver, you ALWAYS have to disable it because it IS your interrupt. Storing the enabled/disabled status in a variable is not needed at all here. Or am I missing something? > > > > priv->uioinfo = uioinfo; > > > spin_lock_init(>lock); > > > - priv->flags = 0; /* interrupt is enabled to begin with */ > > > + /* interrupt is enabled to begin with */ > > > + priv->flags = uioinfo_alloced ? (1 << UIO_INFO_ALLOCED) : 0; > > > > The comment doesn't describe the line it's supposed to comment. > > > > The comment draws attention to the fact that UIO_IRQ_DISABLED is not set > initially, and this is done on purpose. And that's right because the interrupt is initially enabled by uio_register_device(). > Particularly it is done because of > the potential problem you noted earlier - if userspace is never setting this > flag, the interrupt handler will disable irq on the first invocation thanks > to the absence of the flag (if(!test_and_set_bit(UIO_IRQ_DISABLED,...) will > succeed). > So, I think we really need some comment explaining how the things are > arranged > here; after all even you haven't got the whole picture on the first glance. > Maybe the current wording of the comment is not the best. I've just left what > was there before. OK, but we humans tend to read a text from top to bottom, so a comment should describe the code immediately following it. And that is a line that deals with memory allocation, not with interrupts. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Thu, Dec 06, 2012 at 11:11:38AM +0200, Vitalii Demianets wrote: On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote: @@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info) * remember the state so we can allow user space to enable it later. */ - if (!test_and_set_bit(0, priv-flags)) + if (!test_and_set_bit(UIO_IRQ_DISABLED, priv-flags)) disable_irq_nosync(irq); That is not safe. That flag can only be changed by userspace, and if the userspace part is not running, the CPU on which the irq is pending will hang. The handler has to disable the interrupt in any case. (The original version had the same problem, I know...) Perhaps I'm missing something here, but I don't see your point. If userspace is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by writing to the device file, then on the first invocation of interrupt handler that IRQ is disabled by disable_irq_nosync() and that's the end of story - no more irqs, no problems. Until userspace writes non-zero to the device file, of course. If you run into the irq handler, the interrupt was obviously enabled. Since irq sharing is not supported by this driver, you ALWAYS have to disable it because it IS your interrupt. Storing the enabled/disabled status in a variable is not needed at all here. Or am I missing something? priv-uioinfo = uioinfo; spin_lock_init(priv-lock); - priv-flags = 0; /* interrupt is enabled to begin with */ + /* interrupt is enabled to begin with */ + priv-flags = uioinfo_alloced ? (1 UIO_INFO_ALLOCED) : 0; The comment doesn't describe the line it's supposed to comment. The comment draws attention to the fact that UIO_IRQ_DISABLED is not set initially, and this is done on purpose. And that's right because the interrupt is initially enabled by uio_register_device(). Particularly it is done because of the potential problem you noted earlier - if userspace is never setting this flag, the interrupt handler will disable irq on the first invocation thanks to the absence of the flag (if(!test_and_set_bit(UIO_IRQ_DISABLED,...) will succeed). So, I think we really need some comment explaining how the things are arranged here; after all even you haven't got the whole picture on the first glance. Maybe the current wording of the comment is not the best. I've just left what was there before. OK, but we humans tend to read a text from top to bottom, so a comment should describe the code immediately following it. And that is a line that deals with memory allocation, not with interrupts. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] uio: avoid module unloding of in module created UIO devices
On Thu, Dec 06, 2012 at 01:44:57PM +0100, Benedikt Spranger wrote:
A kernel module can create a uio device. Get a reference to the module, if the
UIO device is in use. Otherwise the device can be removed and a uio write or
an
access to am mmaped memory can cause a kernel Oops or other strange effects.
Signed-off-by: Benedikt Spranger b.spran...@linutronix.de
---
drivers/uio/uio.c |8
include/linux/uio_driver.h |1 +
2 files changed, 9 insertions(+)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index b96499a..3b62da0 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -446,6 +446,7 @@ static void uio_release_listener(struct kref *kref)
idev-info-release(idev-info);
module_put(idev-owner);
+ module_put(idev-info-owner);
kfree(listener);
}
@@ -468,6 +469,11 @@ static int uio_open(struct inode *inode, struct file
*filep)
goto out;
}
+ if (!try_module_get(idev-info-owner)) {
+ ret = -ENODEV;
+ goto err_get_module;
+ }
+
listener = kmalloc(sizeof(*listener), GFP_KERNEL);
if (!listener) {
ret = -ENOMEM;
@@ -493,6 +499,8 @@ err_infoopen:
err_alloc_listener:
module_put(idev-owner);
+err_get_module:
+ module_put(idev-owner);
out:
return ret;
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 1bc6493..2862de23 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -95,6 +95,7 @@ struct uio_info {
int (*open)(struct uio_info *info);
void (*release)(struct uio_info *info);
int (*irqcontrol)(struct uio_info *info, s32 irq_on);
+ struct module *owner;
Where is that needed? I won't add a new element to struct uio_info without
an in-kernel user.
Thanks,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] uio: be aware of open(), mmap(), close()
On Thu, Dec 06, 2012 at 01:44:56PM +0100, Benedikt Spranger wrote:
After open(), mmap(), close() the mmaped pointer is valid. Removing the
underlaying module causes a Oops or other strange effects. Keep all structures
valid till the last user is gone.
Signed-off-by: Benedikt Spranger b.spran...@linutronix.de
---
drivers/uio/uio.c | 59
++---
drivers/uio/uio_pdrv_genirq.c |4 +--
include/linux/uio_driver.h|4 +--
3 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..b96499a 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -433,8 +433,22 @@ static irqreturn_t uio_interrupt(int irq, void *dev_id)
struct uio_listener {
struct uio_device *dev;
s32 event_count;
+ struct kref kref;
};
+static void uio_release_listener(struct kref *kref)
+{
+ struct uio_listener *listener = container_of(kref, struct uio_listener,
+ kref);
+ struct uio_device *idev = listener-dev;
+
+ if (idev-info-release)
+ idev-info-release(idev-info);
+
+ module_put(idev-owner);
+ kfree(listener);
+}
+
static int uio_open(struct inode *inode, struct file *filep)
{
struct uio_device *idev;
@@ -465,10 +479,13 @@ static int uio_open(struct inode *inode, struct file
*filep)
filep-private_data = listener;
if (idev-info-open) {
- ret = idev-info-open(idev-info, inode);
+ ret = idev-info-open(idev-info);
if (ret)
goto err_infoopen;
}
+
+ kref_init(listener-kref);
+
return 0;
err_infoopen:
@@ -491,16 +508,11 @@ static int uio_fasync(int fd, struct file *filep, int
on)
static int uio_release(struct inode *inode, struct file *filep)
{
- int ret = 0;
struct uio_listener *listener = filep-private_data;
- struct uio_device *idev = listener-dev;
- if (idev-info-release)
- ret = idev-info-release(idev-info, inode);
+ kref_put(listener-kref, uio_release_listener);
- module_put(idev-owner);
- kfree(listener);
- return ret;
+ return 0;
}
static unsigned int uio_poll(struct file *filep, poll_table *wait)
@@ -605,19 +617,22 @@ static int uio_find_mem_index(struct vm_area_struct
*vma)
static void uio_vma_open(struct vm_area_struct *vma)
{
- struct uio_device *idev = vma-vm_private_data;
- idev-vma_count++;
+ struct uio_listener *listener = vma-vm_private_data;
+
+ kref_get(listener-kref);
}
static void uio_vma_close(struct vm_area_struct *vma)
{
- struct uio_device *idev = vma-vm_private_data;
- idev-vma_count--;
+ struct uio_listener *listener = vma-vm_private_data;
+
+ kref_put(listener-kref, uio_release_listener);
}
static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
{
- struct uio_device *idev = vma-vm_private_data;
+ struct uio_listener *listener = vma-vm_private_data;
+ struct uio_device *idev = listener-dev;
struct page *page;
unsigned long offset;
@@ -646,20 +661,34 @@ static const struct vm_operations_struct uio_vm_ops = {
.fault = uio_vma_fault,
};
+static const struct vm_operations_struct uio_phys_ops = {
+ .open = uio_vma_open,
+ .close = uio_vma_close,
+};
+
static int uio_mmap_physical(struct vm_area_struct *vma)
{
struct uio_device *idev = vma-vm_private_data;
int mi = uio_find_mem_index(vma);
+ int ret;
+
if (mi 0)
return -EINVAL;
vma-vm_page_prot = pgprot_noncached(vma-vm_page_prot);
+ vma-vm_ops = uio_phys_ops;
- return remap_pfn_range(vma,
+ ret = remap_pfn_range(vma,
vma-vm_start,
idev-info-mem[mi].addr PAGE_SHIFT,
vma-vm_end - vma-vm_start,
vma-vm_page_prot);
+ if (ret)
+ return ret;
+
+ uio_vma_open(vma);
+
+ return 0;
}
static int uio_mmap_logical(struct vm_area_struct *vma)
@@ -681,7 +710,7 @@ static int uio_mmap(struct file *filep, struct
vm_area_struct *vma)
if (vma-vm_end vma-vm_start)
return -EINVAL;
- vma-vm_private_data = idev;
+ vma-vm_private_data = listener;
mi = uio_find_mem_index(vma);
if (mi 0)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..a4e32fa 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -37,7 +37,7 @@ struct uio_pdrv_genirq_platdata {
struct platform_device *pdev;
};
-static int uio_pdrv_genirq_open(struct uio_info *info, struct inode *inode)
+static int uio_pdrv_genirq_open(struct uio_info *info)
{
struct
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Wed, Dec 05, 2012 at 11:22:57AM +0200, Vitalii Demianets wrote:
> 1. uioinfo was kfreed based on the presence of pdev->dev.of_node, which was
> obviously wrong and unrelated to the fact if uioinfo was allocated statically
> or dynamically. This patch introduces new flag which clearly shows if uioinfo
> was allocated dynamically and kfrees uioinfo based on that flag;
> 2. Fix: priv data was not freed in case platform_get_irq() failed. As it was
> caused mainly by improper exit labels naming, labels were renamed too;
> 3. The case of uioinfo AND pdev->dev.of_node both NULL (not initialized
> in platform data) was not treated properly.
I noticed two more issues. See below.
>
> Signed-off-by: Vitalii Demianets
>
> ---
> v2: Constants naming style
>
> ---
> drivers/uio/uio_pdrv_genirq.c | 44
> 1 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 42202cd..75aaeee 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -30,6 +30,11 @@
>
> #define DRIVER_NAME "uio_pdrv_genirq"
>
> +enum {
> + UIO_IRQ_DISABLED = 0,
> + UIO_INFO_ALLOCED = 1,
> +};
> +
> struct uio_pdrv_genirq_platdata {
> struct uio_info *uioinfo;
> spinlock_t lock;
> @@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct
> uio_info *dev_info)
>* remember the state so we can allow user space to enable it later.
>*/
>
> - if (!test_and_set_bit(0, >flags))
> + if (!test_and_set_bit(UIO_IRQ_DISABLED, >flags))
> disable_irq_nosync(irq);
That is not safe. That flag can only be changed by userspace, and if the
userspace part is not running, the CPU on which the irq is pending will hang.
The handler has to disable the interrupt in any case.
(The original version had the same problem, I know...)
>
> return IRQ_HANDLED;
> @@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info
> *dev_info, s32 irq_on)
>
> spin_lock_irqsave(>lock, flags);
> if (irq_on) {
> - if (test_and_clear_bit(0, >flags))
> + if (test_and_clear_bit(UIO_IRQ_DISABLED, >flags))
> enable_irq(dev_info->irq);
> } else {
> - if (!test_and_set_bit(0, >flags))
> + if (!test_and_set_bit(UIO_IRQ_DISABLED, >flags))
> disable_irq(dev_info->irq);
> }
> spin_unlock_irqrestore(>lock, flags);
> @@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> struct uio_mem *uiomem;
> int ret = -EINVAL;
> int i;
> + bool uioinfo_alloced = false;
>
> - if (!uioinfo) {
> + if (!uioinfo && pdev->dev.of_node) {
> int irq;
>
> /* alloc uioinfo for one device */
> @@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> if (!uioinfo) {
> ret = -ENOMEM;
> dev_err(>dev, "unable to kmalloc\n");
> - goto bad2;
> + goto out;
> }
> uioinfo->name = pdev->dev.of_node->name;
> uioinfo->version = "devicetree";
> + uioinfo_alloced = true;
>
> /* Multiple IRQs are not supported */
> irq = platform_get_irq(pdev, 0);
> @@ -125,32 +132,33 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
>
> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> dev_err(>dev, "missing platform_data\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> if (uioinfo->handler || uioinfo->irqcontrol ||
> uioinfo->irq_flags & IRQF_SHARED) {
> dev_err(>dev, "interrupt configuration error\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> if (!priv) {
> ret = -ENOMEM;
> dev_err(>dev, "unable to kmalloc\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv->uioinfo = uioinfo;
> spin_lock_init(>lock);
> - priv->flags = 0; /* interrupt is enabled to begin with */
> + /* interrupt is enabled to begin with */
> + priv->flags = uioinfo_alloced ? (1 << UIO_INFO_ALLOCED) : 0;
The comment doesn't describe the line it's supposed to comment.
> priv->pdev = pdev;
>
> if (!uioinfo->irq) {
> ret = platform_get_irq(pdev, 0);
> if (ret < 0) {
> dev_err(>dev, "failed to get IRQ\n");
> - goto bad0;
> + goto out_priv;
> }
> uioinfo->irq = ret;
> }
> @@ -205,19 +213,19 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> ret =
Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Wed, Dec 05, 2012 at 11:22:57AM +0200, Vitalii Demianets wrote:
1. uioinfo was kfreed based on the presence of pdev-dev.of_node, which was
obviously wrong and unrelated to the fact if uioinfo was allocated statically
or dynamically. This patch introduces new flag which clearly shows if uioinfo
was allocated dynamically and kfrees uioinfo based on that flag;
2. Fix: priv data was not freed in case platform_get_irq() failed. As it was
caused mainly by improper exit labels naming, labels were renamed too;
3. The case of uioinfo AND pdev-dev.of_node both NULL (not initialized
in platform data) was not treated properly.
I noticed two more issues. See below.
Signed-off-by: Vitalii Demianets vi...@nppfactor.kiev.ua
---
v2: Constants naming style
---
drivers/uio/uio_pdrv_genirq.c | 44
1 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..75aaeee 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -30,6 +30,11 @@
#define DRIVER_NAME uio_pdrv_genirq
+enum {
+ UIO_IRQ_DISABLED = 0,
+ UIO_INFO_ALLOCED = 1,
+};
+
struct uio_pdrv_genirq_platdata {
struct uio_info *uioinfo;
spinlock_t lock;
@@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct
uio_info *dev_info)
* remember the state so we can allow user space to enable it later.
*/
- if (!test_and_set_bit(0, priv-flags))
+ if (!test_and_set_bit(UIO_IRQ_DISABLED, priv-flags))
disable_irq_nosync(irq);
That is not safe. That flag can only be changed by userspace, and if the
userspace part is not running, the CPU on which the irq is pending will hang.
The handler has to disable the interrupt in any case.
(The original version had the same problem, I know...)
return IRQ_HANDLED;
@@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info
*dev_info, s32 irq_on)
spin_lock_irqsave(priv-lock, flags);
if (irq_on) {
- if (test_and_clear_bit(0, priv-flags))
+ if (test_and_clear_bit(UIO_IRQ_DISABLED, priv-flags))
enable_irq(dev_info-irq);
} else {
- if (!test_and_set_bit(0, priv-flags))
+ if (!test_and_set_bit(UIO_IRQ_DISABLED, priv-flags))
disable_irq(dev_info-irq);
}
spin_unlock_irqrestore(priv-lock, flags);
@@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
struct uio_mem *uiomem;
int ret = -EINVAL;
int i;
+ bool uioinfo_alloced = false;
- if (!uioinfo) {
+ if (!uioinfo pdev-dev.of_node) {
int irq;
/* alloc uioinfo for one device */
@@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
if (!uioinfo) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad2;
+ goto out;
}
uioinfo-name = pdev-dev.of_node-name;
uioinfo-version = devicetree;
+ uioinfo_alloced = true;
/* Multiple IRQs are not supported */
irq = platform_get_irq(pdev, 0);
@@ -125,32 +132,33 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
if (!uioinfo || !uioinfo-name || !uioinfo-version) {
dev_err(pdev-dev, missing platform_data\n);
- goto bad0;
+ goto out_uioinfo;
}
if (uioinfo-handler || uioinfo-irqcontrol ||
uioinfo-irq_flags IRQF_SHARED) {
dev_err(pdev-dev, interrupt configuration error\n);
- goto bad0;
+ goto out_uioinfo;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad0;
+ goto out_uioinfo;
}
priv-uioinfo = uioinfo;
spin_lock_init(priv-lock);
- priv-flags = 0; /* interrupt is enabled to begin with */
+ /* interrupt is enabled to begin with */
+ priv-flags = uioinfo_alloced ? (1 UIO_INFO_ALLOCED) : 0;
The comment doesn't describe the line it's supposed to comment.
priv-pdev = pdev;
if (!uioinfo-irq) {
ret = platform_get_irq(pdev, 0);
if (ret 0) {
dev_err(pdev-dev, failed to get IRQ\n);
- goto bad0;
+ goto out_priv;
}
uioinfo-irq = ret;
}
@@ -205,19 +213,19 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
ret = uio_register_device(pdev-dev, priv-uioinfo);
if (ret) {
dev_err(pdev-dev,
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Thu, Nov 29, 2012 at 01:47:28PM +0200, Vitalii Demianets wrote:
> 1. uioinfo was kfreed based on the presence of pdev->dev.of_node, which was
> obviously wrong and unrelated to the fact if uioinfo was allocated statically
> or dynamically. This patch introduces new flag which clearly shows if uioinfo
> was allocated dynamically and kfrees uioinfo based on that flag;
> 2. Fix: priv data was not freed in case platform_get_irq() failed. As it was
> caused mainly by improper exit labels naming, labels were renamed too;
> 3. The case of uioinfo AND pdev->dev.of_node both NULL (not initialized
> in platform data) was not treated properly.
Your arguments sound right to me. But there are some minor issues, see below.
Otherwise looks good.
Thanks,
Hans
>
> Signed-off-by: Vitalii Demianets
> ---
> drivers/uio/uio_pdrv_genirq.c | 44
> 1 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 42202cd..45126e3 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -30,6 +30,11 @@
>
> #define DRIVER_NAME "uio_pdrv_genirq"
>
> +enum {
> + bitIRQDisabled = 0,
> + bitUioinfoAlloced = 1,
Please do not use CamelCase. This is neither Windows nor C++ or Java.
And you don't need to tell users about the type (like "bit"). That should
be clear by looking at the code. I'd prefer UPPERCASE to make it clear that
these are constants, e.g. IRQ_IS_DISABLED and UIO_INFO_IS_ALLOCATED or
something like that.
> +};
> +
> struct uio_pdrv_genirq_platdata {
> struct uio_info *uioinfo;
> spinlock_t lock;
> @@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct
> uio_info *dev_info)
>* remember the state so we can allow user space to enable it later.
>*/
>
> - if (!test_and_set_bit(0, >flags))
> + if (!test_and_set_bit(bitIRQDisabled, >flags))
> disable_irq_nosync(irq);
>
> return IRQ_HANDLED;
> @@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info
> *dev_info, s32 irq_on)
>
> spin_lock_irqsave(>lock, flags);
> if (irq_on) {
> - if (test_and_clear_bit(0, >flags))
> + if (test_and_clear_bit(bitIRQDisabled, >flags))
> enable_irq(dev_info->irq);
> } else {
> - if (!test_and_set_bit(0, >flags))
> + if (!test_and_set_bit(bitIRQDisabled, >flags))
> disable_irq(dev_info->irq);
> }
> spin_unlock_irqrestore(>lock, flags);
> @@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> struct uio_mem *uiomem;
> int ret = -EINVAL;
> int i;
> + bool uioinfo_alloced = false;
>
> - if (!uioinfo) {
> + if (!uioinfo && pdev->dev.of_node) {
> int irq;
>
> /* alloc uioinfo for one device */
> @@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> if (!uioinfo) {
> ret = -ENOMEM;
> dev_err(>dev, "unable to kmalloc\n");
> - goto bad2;
> + goto out;
> }
> uioinfo->name = pdev->dev.of_node->name;
> uioinfo->version = "devicetree";
> + uioinfo_alloced = true;
>
> /* Multiple IRQs are not supported */
> irq = platform_get_irq(pdev, 0);
> @@ -125,32 +132,33 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
>
> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> dev_err(>dev, "missing platform_data\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> if (uioinfo->handler || uioinfo->irqcontrol ||
> uioinfo->irq_flags & IRQF_SHARED) {
> dev_err(>dev, "interrupt configuration error\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> if (!priv) {
> ret = -ENOMEM;
> dev_err(>dev, "unable to kmalloc\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv->uioinfo = uioinfo;
> spin_lock_init(>lock);
> - priv->flags = 0; /* interrupt is enabled to begin with */
> + /* interrupt is enabled to begin with */
> + priv->flags = uioinfo_alloced ? (1 << bitUioinfoAlloced) : 0;
> priv->pdev = pdev;
>
> if (!uioinfo->irq) {
> ret = platform_get_irq(pdev, 0);
> if (ret < 0) {
> dev_err(>dev, "failed to get IRQ\n");
> - goto bad0;
> + goto out_priv;
> }
> uioinfo->irq = ret;
> }
> @@ -205,19 +213,19 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> ret
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
On Thu, Nov 29, 2012 at 01:47:28PM +0200, Vitalii Demianets wrote:
1. uioinfo was kfreed based on the presence of pdev-dev.of_node, which was
obviously wrong and unrelated to the fact if uioinfo was allocated statically
or dynamically. This patch introduces new flag which clearly shows if uioinfo
was allocated dynamically and kfrees uioinfo based on that flag;
2. Fix: priv data was not freed in case platform_get_irq() failed. As it was
caused mainly by improper exit labels naming, labels were renamed too;
3. The case of uioinfo AND pdev-dev.of_node both NULL (not initialized
in platform data) was not treated properly.
Your arguments sound right to me. But there are some minor issues, see below.
Otherwise looks good.
Thanks,
Hans
Signed-off-by: Vitalii Demianets vi...@nppfactor.kiev.ua
---
drivers/uio/uio_pdrv_genirq.c | 44
1 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..45126e3 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -30,6 +30,11 @@
#define DRIVER_NAME uio_pdrv_genirq
+enum {
+ bitIRQDisabled = 0,
+ bitUioinfoAlloced = 1,
Please do not use CamelCase. This is neither Windows nor C++ or Java.
And you don't need to tell users about the type (like bit). That should
be clear by looking at the code. I'd prefer UPPERCASE to make it clear that
these are constants, e.g. IRQ_IS_DISABLED and UIO_INFO_IS_ALLOCATED or
something like that.
+};
+
struct uio_pdrv_genirq_platdata {
struct uio_info *uioinfo;
spinlock_t lock;
@@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct
uio_info *dev_info)
* remember the state so we can allow user space to enable it later.
*/
- if (!test_and_set_bit(0, priv-flags))
+ if (!test_and_set_bit(bitIRQDisabled, priv-flags))
disable_irq_nosync(irq);
return IRQ_HANDLED;
@@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info
*dev_info, s32 irq_on)
spin_lock_irqsave(priv-lock, flags);
if (irq_on) {
- if (test_and_clear_bit(0, priv-flags))
+ if (test_and_clear_bit(bitIRQDisabled, priv-flags))
enable_irq(dev_info-irq);
} else {
- if (!test_and_set_bit(0, priv-flags))
+ if (!test_and_set_bit(bitIRQDisabled, priv-flags))
disable_irq(dev_info-irq);
}
spin_unlock_irqrestore(priv-lock, flags);
@@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
struct uio_mem *uiomem;
int ret = -EINVAL;
int i;
+ bool uioinfo_alloced = false;
- if (!uioinfo) {
+ if (!uioinfo pdev-dev.of_node) {
int irq;
/* alloc uioinfo for one device */
@@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
if (!uioinfo) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad2;
+ goto out;
}
uioinfo-name = pdev-dev.of_node-name;
uioinfo-version = devicetree;
+ uioinfo_alloced = true;
/* Multiple IRQs are not supported */
irq = platform_get_irq(pdev, 0);
@@ -125,32 +132,33 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
if (!uioinfo || !uioinfo-name || !uioinfo-version) {
dev_err(pdev-dev, missing platform_data\n);
- goto bad0;
+ goto out_uioinfo;
}
if (uioinfo-handler || uioinfo-irqcontrol ||
uioinfo-irq_flags IRQF_SHARED) {
dev_err(pdev-dev, interrupt configuration error\n);
- goto bad0;
+ goto out_uioinfo;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad0;
+ goto out_uioinfo;
}
priv-uioinfo = uioinfo;
spin_lock_init(priv-lock);
- priv-flags = 0; /* interrupt is enabled to begin with */
+ /* interrupt is enabled to begin with */
+ priv-flags = uioinfo_alloced ? (1 bitUioinfoAlloced) : 0;
priv-pdev = pdev;
if (!uioinfo-irq) {
ret = platform_get_irq(pdev, 0);
if (ret 0) {
dev_err(pdev-dev, failed to get IRQ\n);
- goto bad0;
+ goto out_priv;
}
uioinfo-irq = ret;
}
@@ -205,19 +213,19 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
ret = uio_register_device(pdev-dev, priv-uioinfo);
if (ret) {
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Mon, Dec 03, 2012 at 10:53:45AM +0200, Vitalii Demianets wrote: > > On Friday 30 November 2012 23:39:06 Hans J. Koch wrote: > > > Thanks a lot for reporting and discussing that problem. I'll add a > > > > > > Reported-by: Vitalii Demianets > > > > > > if you have no objections. > > > > No objections. Thanks, Hans. > > By the way, what do you think about my revised version of uio_pdrv_genirq.c > patch from November 29th? I hope I find the time to look at that later tonight. I'm a bit busy ATM. Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Mon, Dec 03, 2012 at 10:53:45AM +0200, Vitalii Demianets wrote: On Friday 30 November 2012 23:39:06 Hans J. Koch wrote: Thanks a lot for reporting and discussing that problem. I'll add a Reported-by: Vitalii Demianets vi...@nppfactor.kiev.ua if you have no objections. No objections. Thanks, Hans. By the way, what do you think about my revised version of uio_pdrv_genirq.c patch from November 29th? I hope I find the time to look at that later tonight. I'm a bit busy ATM. Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Sat, Dec 01, 2012 at 09:58:32AM +, Cong Ding wrote: > If it is really necessary to save the 4 lines of codes, I would suggest to do > in the following style. But you are more senior than me, so I may be wrong in > this aspect. "Seniority" (whatever you mean by that) has got nothing to do with it. I'll accept any child's patch if it is good. Discussing code on LKML is a purely technically thing, only if two patches are technically equivalent, the maintainer's personal taste will decide. And if it makes you feel better: My patches (and those of any other developer) are also sometimes rejected because they are not to some other maintainer's taste. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] stable: uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable "ret" can be undefined.
Signed-off-by: Hans J. Koch
Reported-by: Vitalii Demianets
---
drivers/uio/uio.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..0c80df2 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
struct uio_portio *portio;
for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
+ ret = -ENOMEM;
mem = >info->mem[mi];
if (mem->size == 0)
break;
@@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
}
for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
+ ret = -ENOMEM;
port = >info->port[pi];
if (port->size == 0)
break;
--
1.7.9
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] stable: uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable ret can be undefined.
Signed-off-by: Hans J. Koch h...@hansjkoch.de
Reported-by: Vitalii Demianets vi...@nppfactor.kiev.ua
---
drivers/uio/uio.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..0c80df2 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
struct uio_portio *portio;
for (mi = 0; mi MAX_UIO_MAPS; mi++) {
+ ret = -ENOMEM;
mem = idev-info-mem[mi];
if (mem-size == 0)
break;
@@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
}
for (pi = 0; pi MAX_UIO_PORT_REGIONS; pi++) {
+ ret = -ENOMEM;
port = idev-info-port[pi];
if (port-size == 0)
break;
--
1.7.9
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Sat, Dec 01, 2012 at 09:58:32AM +, Cong Ding wrote: If it is really necessary to save the 4 lines of codes, I would suggest to do in the following style. But you are more senior than me, so I may be wrong in this aspect. Seniority (whatever you mean by that) has got nothing to do with it. I'll accept any child's patch if it is good. Discussing code on LKML is a purely technically thing, only if two patches are technically equivalent, the maintainer's personal taste will decide. And if it makes you feel better: My patches (and those of any other developer) are also sometimes rejected because they are not to some other maintainer's taste. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Sat, Dec 01, 2012 at 02:22:44AM +0100, Cong Ding wrote: > On Fri, Nov 30, 2012 at 10:33 PM, Hans J. Koch wrote: > > On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote: > >> I like Vitalii's solution more. Hans's solution assign the value > >> -ENOMEM to ret in every round of the loop, which is a kind of wasting > >> CPU cycles. > > > > The difference between > > 1 files changed, 12 insertions(+), 4 deletions(-) and > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > is more important. Note that this code is not in a fast path but only > > called once at device creation. > Why do you think the size of the patch is so important? I think the > most important thing is the coding style and efficiency. I agree > efficiency is not important in this case, but what about coding style? Most important. Short is beautiful. > Your code is _not_ very easy to understand. You think so? > It's a very natural thing > to set the return value and then goto the error-handling codes, which > is exactly same as what Vitalii did, rather than setting an initial > value in the beginning of each round of the loop as you did. Setting a default value at the beginning of a block is the most natural thing. I don't want to repeat the same code in three places. > There are > also a bunch of codes in kernel in the same style with Vitalii, but I > cannot find anything same as your codes. If you follow LKML closely, you'll notice that simplifying code by replacing unnecessary repetitions with shorter versions is always welcome. If we didn't go for that, the kernel source would be a few million lines bigger by now. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Fri, Nov 30, 2012 at 01:16:19PM +0200, Vitalii Demianets wrote:
> On Friday 30 November 2012 01:58:22 Hans J. Koch wrote:
> > On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
> > > > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> > > > > Hans, I think there are something wrong in your patch, while
> > > > > Vitalii's is right. The variable "ret" is reused in line 292 and line
> > > > > 295, so the value of "ret" would be overridden (if it goto err_map in
> > > > > line 284 when mi>=1).
> > > >
> > > > Actually, both patches do exactly the same thing. Hans's patch
> > > > establishes default value for the ret for all those "other" cases when
> > > > ret is not explicitly overridden. My patch explicitly enumerates all
> > > > those "other" cases in more wordily manner.
> > >
> > > Oops, disregard this. After looking at it more thoroughly I got your
> > > point. You are right, ret is overridden at first iteration (mi == 0), so
> > > Hans's approach does not work.
> > > I must do more thinking before replying in a hurry.
> >
> > You're right. Initialization of "ret" has to take place at the beginning of
> > the loop.
> >
> > I think this version is right:
>
> Yes, this looks right for me.
OK, I'll send that patch offically, then. This might also be material for
the stable updates. Greg?
Thanks a lot for reporting and discussing that problem. I'll add a
Reported-by: Vitalii Demianets
if you have no objections.
Thanks,
Hans
>
> > >From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
> >
> > From: "Hans J. Koch"
> > Date: Fri, 30 Nov 2012 00:51:50 +0100
> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> >
> > In two cases, the return value variable "ret" can be undefined.
> >
> > Signed-off-by: Hans J. Koch
> > ---
> > drivers/uio/uio.c |2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > index 5110f36..0c80df2 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device
> > *idev) struct uio_portio *portio;
> >
> > for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> > + ret = -ENOMEM;
> > mem = >info->mem[mi];
> > if (mem->size == 0)
> > break;
> > @@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device
> > *idev) }
> >
> > for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
> > + ret = -ENOMEM;
> > port = >info->port[pi];
> > if (port->size == 0)
> > break;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote:
> I like Vitalii's solution more. Hans's solution assign the value
> -ENOMEM to ret in every round of the loop, which is a kind of wasting
> CPU cycles.
The difference between
1 files changed, 12 insertions(+), 4 deletions(-) and
1 files changed, 2 insertions(+), 0 deletions(-)
is more important. Note that this code is not in a fast path but only
called once at device creation.
Thanks,
Hans
>
> On Fri, Nov 30, 2012 at 12:58 AM, Hans J. Koch wrote:
> > On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
> >> > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> >> > > Hans, I think there are something wrong in your patch, while Vitalii's
> >> > > is right. The variable "ret" is reused in line 292 and line 295, so
> >> > > the value of "ret" would be overridden (if it goto err_map in line 284
> >> > > when mi>=1).
> >> >
> >> > Actually, both patches do exactly the same thing. Hans's patch
> >> > establishes
> >> > default value for the ret for all those "other" cases when ret is not
> >> > explicitly overridden. My patch explicitly enumerates all those "other"
> >> > cases in more wordily manner.
> >> >
> >>
> >> Oops, disregard this. After looking at it more thoroughly I got your point.
> >> You are right, ret is overridden at first iteration (mi == 0), so Hans's
> >> approach does not work.
> >> I must do more thinking before replying in a hurry.
> >
> > You're right. Initialization of "ret" has to take place at the beginning of
> > the loop.
> >
> > I think this version is right:
> >
> >
> > From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
> > From: "Hans J. Koch"
> > Date: Fri, 30 Nov 2012 00:51:50 +0100
> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> >
> > In two cases, the return value variable "ret" can be undefined.
> >
> > Signed-off-by: Hans J. Koch
> > ---
> > drivers/uio/uio.c |2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > index 5110f36..0c80df2 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device
> > *idev)
> > struct uio_portio *portio;
> >
> > for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> > + ret = -ENOMEM;
> > mem = >info->mem[mi];
> > if (mem->size == 0)
> > break;
> > @@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device
> > *idev)
> > }
> >
> > for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
> > + ret = -ENOMEM;
> > port = >info->port[pi];
> > if (port->size == 0)
> > break;
> > --
> > 1.7.9
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote:
I like Vitalii's solution more. Hans's solution assign the value
-ENOMEM to ret in every round of the loop, which is a kind of wasting
CPU cycles.
The difference between
1 files changed, 12 insertions(+), 4 deletions(-) and
1 files changed, 2 insertions(+), 0 deletions(-)
is more important. Note that this code is not in a fast path but only
called once at device creation.
Thanks,
Hans
On Fri, Nov 30, 2012 at 12:58 AM, Hans J. Koch h...@hansjkoch.de wrote:
On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
On Thursday 29 November 2012 18:05:27 Tux9 wrote:
Hans, I think there are something wrong in your patch, while Vitalii's
is right. The variable ret is reused in line 292 and line 295, so
the value of ret would be overridden (if it goto err_map in line 284
when mi=1).
Actually, both patches do exactly the same thing. Hans's patch
establishes
default value for the ret for all those other cases when ret is not
explicitly overridden. My patch explicitly enumerates all those other
cases in more wordily manner.
Oops, disregard this. After looking at it more thoroughly I got your point.
You are right, ret is overridden at first iteration (mi == 0), so Hans's
approach does not work.
I must do more thinking before replying in a hurry.
You're right. Initialization of ret has to take place at the beginning of
the loop.
I think this version is right:
From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
From: Hans J. Koch h...@hansjkoch.de
Date: Fri, 30 Nov 2012 00:51:50 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable ret can be undefined.
Signed-off-by: Hans J. Koch h...@hansjkoch.de
---
drivers/uio/uio.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..0c80df2 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device
*idev)
struct uio_portio *portio;
for (mi = 0; mi MAX_UIO_MAPS; mi++) {
+ ret = -ENOMEM;
mem = idev-info-mem[mi];
if (mem-size == 0)
break;
@@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device
*idev)
}
for (pi = 0; pi MAX_UIO_PORT_REGIONS; pi++) {
+ ret = -ENOMEM;
port = idev-info-port[pi];
if (port-size == 0)
break;
--
1.7.9
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Fri, Nov 30, 2012 at 01:16:19PM +0200, Vitalii Demianets wrote:
On Friday 30 November 2012 01:58:22 Hans J. Koch wrote:
On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
On Thursday 29 November 2012 18:05:27 Tux9 wrote:
Hans, I think there are something wrong in your patch, while
Vitalii's is right. The variable ret is reused in line 292 and line
295, so the value of ret would be overridden (if it goto err_map in
line 284 when mi=1).
Actually, both patches do exactly the same thing. Hans's patch
establishes default value for the ret for all those other cases when
ret is not explicitly overridden. My patch explicitly enumerates all
those other cases in more wordily manner.
Oops, disregard this. After looking at it more thoroughly I got your
point. You are right, ret is overridden at first iteration (mi == 0), so
Hans's approach does not work.
I must do more thinking before replying in a hurry.
You're right. Initialization of ret has to take place at the beginning of
the loop.
I think this version is right:
Yes, this looks right for me.
OK, I'll send that patch offically, then. This might also be material for
the stable updates. Greg?
Thanks a lot for reporting and discussing that problem. I'll add a
Reported-by: Vitalii Demianets vi...@nppfactor.kiev.ua
if you have no objections.
Thanks,
Hans
From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
From: Hans J. Koch h...@hansjkoch.de
Date: Fri, 30 Nov 2012 00:51:50 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable ret can be undefined.
Signed-off-by: Hans J. Koch h...@hansjkoch.de
---
drivers/uio/uio.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..0c80df2 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device
*idev) struct uio_portio *portio;
for (mi = 0; mi MAX_UIO_MAPS; mi++) {
+ ret = -ENOMEM;
mem = idev-info-mem[mi];
if (mem-size == 0)
break;
@@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device
*idev) }
for (pi = 0; pi MAX_UIO_PORT_REGIONS; pi++) {
+ ret = -ENOMEM;
port = idev-info-port[pi];
if (port-size == 0)
break;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Sat, Dec 01, 2012 at 02:22:44AM +0100, Cong Ding wrote: On Fri, Nov 30, 2012 at 10:33 PM, Hans J. Koch h...@hansjkoch.de wrote: On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote: I like Vitalii's solution more. Hans's solution assign the value -ENOMEM to ret in every round of the loop, which is a kind of wasting CPU cycles. The difference between 1 files changed, 12 insertions(+), 4 deletions(-) and 1 files changed, 2 insertions(+), 0 deletions(-) is more important. Note that this code is not in a fast path but only called once at device creation. Why do you think the size of the patch is so important? I think the most important thing is the coding style and efficiency. I agree efficiency is not important in this case, but what about coding style? Most important. Short is beautiful. Your code is _not_ very easy to understand. You think so? It's a very natural thing to set the return value and then goto the error-handling codes, which is exactly same as what Vitalii did, rather than setting an initial value in the beginning of each round of the loop as you did. Setting a default value at the beginning of a block is the most natural thing. I don't want to repeat the same code in three places. There are also a bunch of codes in kernel in the same style with Vitalii, but I cannot find anything same as your codes. If you follow LKML closely, you'll notice that simplifying code by replacing unnecessary repetitions with shorter versions is always welcome. If we didn't go for that, the kernel source would be a few million lines bigger by now. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/1] uio.c: solve memory leak
On Thu, Nov 29, 2012 at 05:40:00PM +, Cong Ding wrote:
> In version 1, I forgot to modify the same bug in the first loop.
>
> we have to call kobject_put() to clean up the kobject after function
> kobject_init(), kobject_add(), or kobject_uevent() is called.
Yes, thanks. Incredible how that code looks like...
There's still another bug: The memory allocated with kzalloc is
never freed. More tomorrow, have to go to sleep now.
Thanks,
Hans
>
> Signed-off-by: Cong Ding
> ---
> drivers/uio/uio.c | 16 ++--
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..79774d3 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -291,10 +291,10 @@ static int uio_dev_add_attributes(struct uio_device
> *idev)
> mem->map = map;
> ret = kobject_add(>kobj, idev->map_dir, "map%d", mi);
> if (ret)
> - goto err_map;
> + goto err_map_kobj;
> ret = kobject_uevent(>kobj, KOBJ_ADD);
> if (ret)
> - goto err_map;
> + goto err_map_kobj;
> }
>
> for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
> @@ -317,23 +317,27 @@ static int uio_dev_add_attributes(struct uio_device
> *idev)
> ret = kobject_add(>kobj, idev->portio_dir,
> "port%d", pi);
> if (ret)
> - goto err_portio;
> + goto err_portio_kobj;
> ret = kobject_uevent(>kobj, KOBJ_ADD);
> if (ret)
> - goto err_portio;
> + goto err_portio_kobj;
> }
>
> return 0;
>
> err_portio:
> - for (pi--; pi >= 0; pi--) {
> + pi--;
> +err_portio_kobj:
> + for (; pi >= 0; pi--) {
> port = >info->port[pi];
> portio = port->portio;
> kobject_put(>kobj);
> }
> kobject_put(idev->portio_dir);
> err_map:
> - for (mi--; mi>=0; mi--) {
> + mi--;
> +err_map_kobj:
> + for (; mi >= 0; mi--) {
> mem = >info->mem[mi];
> map = mem->map;
> kobject_put(>kobj);
> --
> 1.7.4.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
> > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> > > Hans, I think there are something wrong in your patch, while Vitalii's
> > > is right. The variable "ret" is reused in line 292 and line 295, so
> > > the value of "ret" would be overridden (if it goto err_map in line 284
> > > when mi>=1).
> >
> > Actually, both patches do exactly the same thing. Hans's patch establishes
> > default value for the ret for all those "other" cases when ret is not
> > explicitly overridden. My patch explicitly enumerates all those "other"
> > cases in more wordily manner.
> >
>
> Oops, disregard this. After looking at it more thoroughly I got your point.
> You are right, ret is overridden at first iteration (mi == 0), so Hans's
> approach does not work.
> I must do more thinking before replying in a hurry.
You're right. Initialization of "ret" has to take place at the beginning of
the loop.
I think this version is right:
>From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
From: "Hans J. Koch"
Date: Fri, 30 Nov 2012 00:51:50 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable "ret" can be undefined.
Signed-off-by: Hans J. Koch
---
drivers/uio/uio.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..0c80df2 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
struct uio_portio *portio;
for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
+ ret = -ENOMEM;
mem = >info->mem[mi];
if (mem->size == 0)
break;
@@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
}
for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
+ ret = -ENOMEM;
port = >info->port[pi];
if (port->size == 0)
break;
--
1.7.9
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
On Thursday 29 November 2012 18:05:27 Tux9 wrote:
Hans, I think there are something wrong in your patch, while Vitalii's
is right. The variable ret is reused in line 292 and line 295, so
the value of ret would be overridden (if it goto err_map in line 284
when mi=1).
Actually, both patches do exactly the same thing. Hans's patch establishes
default value for the ret for all those other cases when ret is not
explicitly overridden. My patch explicitly enumerates all those other
cases in more wordily manner.
Oops, disregard this. After looking at it more thoroughly I got your point.
You are right, ret is overridden at first iteration (mi == 0), so Hans's
approach does not work.
I must do more thinking before replying in a hurry.
You're right. Initialization of ret has to take place at the beginning of
the loop.
I think this version is right:
From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
From: Hans J. Koch h...@hansjkoch.de
Date: Fri, 30 Nov 2012 00:51:50 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable ret can be undefined.
Signed-off-by: Hans J. Koch h...@hansjkoch.de
---
drivers/uio/uio.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..0c80df2 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
struct uio_portio *portio;
for (mi = 0; mi MAX_UIO_MAPS; mi++) {
+ ret = -ENOMEM;
mem = idev-info-mem[mi];
if (mem-size == 0)
break;
@@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
}
for (pi = 0; pi MAX_UIO_PORT_REGIONS; pi++) {
+ ret = -ENOMEM;
port = idev-info-port[pi];
if (port-size == 0)
break;
--
1.7.9
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/1] uio.c: solve memory leak
On Thu, Nov 29, 2012 at 05:40:00PM +, Cong Ding wrote:
In version 1, I forgot to modify the same bug in the first loop.
we have to call kobject_put() to clean up the kobject after function
kobject_init(), kobject_add(), or kobject_uevent() is called.
Yes, thanks. Incredible how that code looks like...
There's still another bug: The memory allocated with kzalloc is
never freed. More tomorrow, have to go to sleep now.
Thanks,
Hans
Signed-off-by: Cong Ding ding...@gmail.com
---
drivers/uio/uio.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..79774d3 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -291,10 +291,10 @@ static int uio_dev_add_attributes(struct uio_device
*idev)
mem-map = map;
ret = kobject_add(map-kobj, idev-map_dir, map%d, mi);
if (ret)
- goto err_map;
+ goto err_map_kobj;
ret = kobject_uevent(map-kobj, KOBJ_ADD);
if (ret)
- goto err_map;
+ goto err_map_kobj;
}
for (pi = 0; pi MAX_UIO_PORT_REGIONS; pi++) {
@@ -317,23 +317,27 @@ static int uio_dev_add_attributes(struct uio_device
*idev)
ret = kobject_add(portio-kobj, idev-portio_dir,
port%d, pi);
if (ret)
- goto err_portio;
+ goto err_portio_kobj;
ret = kobject_uevent(portio-kobj, KOBJ_ADD);
if (ret)
- goto err_portio;
+ goto err_portio_kobj;
}
return 0;
err_portio:
- for (pi--; pi = 0; pi--) {
+ pi--;
+err_portio_kobj:
+ for (; pi = 0; pi--) {
port = idev-info-port[pi];
portio = port-portio;
kobject_put(portio-kobj);
}
kobject_put(idev-portio_dir);
err_map:
- for (mi--; mi=0; mi--) {
+ mi--;
+err_map_kobj:
+ for (; mi = 0; mi--) {
mem = idev-info-mem[mi];
map = mem-map;
kobject_put(map-kobj);
--
1.7.4.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak & confusing labels
On Wed, Nov 28, 2012 at 11:37:23AM +0200, Vitalii Demianets wrote: > On Wednesday 28 November 2012 02:37:50 Hans J. Koch wrote: > > > > In other words, the case of uioinfo AND pdev->dev.of_node both being NULL > > is not handled properly and will have ugly results. > > > > Moreover, the case of (uioinfo != NULL) && (pdev->dev.of_node != NULL) leads > to equally ugly results too (freeing uoinfo when it is statically allocated). You're right. That wants to be fixed as well. > > I think, we should sort out these problems, but in another patch. It is > totally unrelated to the problem solved by the original patch (memory leak > caused by not freeing priv in case platform_get_irq() fails). So far, no patch was applied. I don't mind if all fixes for uio_pdrv_genirq are in one patch as long as only this one file is altered. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak & confusing labels
On Wed, Nov 28, 2012 at 10:29:29AM +0100, Cong Ding wrote:
> On Wed, Nov 28, 2012 at 1:37 AM, Hans J. Koch wrote:
> > On Wed, Nov 28, 2012 at 01:07:26AM +0100, Cong Ding wrote:
> >> On Wed, Nov 28, 2012 at 12:07 AM, Hans J. Koch wrote:
> >> > On Tue, Nov 27, 2012 at 07:29:32PM +0200, Vitalii Demianets wrote:
> >> >> Memory leak was caused by jumping to the wrong exit label. So, it is
> >> >> good time
> >> >> to improve misleading label names too.
> >> >
> >> > I agree that bad0, bad1, and bad2 are not the best choice for label
> >> > names...
> >> > I don't have any objections to your renaming.
> >> >
> >> > But there's a more serious bug, maybe you can fix that as well while
> >> > you're
> >> > at it? (See below)
> >> >
> >> > Thanks,
> >> > Hans
> >> >
> >> >>
> >> >> Signed-off-by: Vitalii Demianets
> >> >> ---
> >> >> drivers/uio/uio_pdrv_genirq.c | 21 +++--
> >> >> 1 files changed, 11 insertions(+), 10 deletions(-)
> >> >>
> >> >> diff --git a/drivers/uio/uio_pdrv_genirq.c
> >> >> b/drivers/uio/uio_pdrv_genirq.c
> >> >> index 42202cd..b88cf7b 100644
> >> >> --- a/drivers/uio/uio_pdrv_genirq.c
> >> >> +++ b/drivers/uio/uio_pdrv_genirq.c
> >> >> @@ -110,7 +110,7 @@ static int uio_pdrv_genirq_probe(struct
> >> >> platform_device *pdev)
> >> >> if (!uioinfo) {
> >> >> ret = -ENOMEM;
> >> >> dev_err(>dev, "unable to kmalloc\n");
> >> >> - goto bad2;
> >> >> + goto out;
> >> >> }
> >> >> uioinfo->name = pdev->dev.of_node->name;
> >> >> uioinfo->version = "devicetree";
> >> >> @@ -125,20 +125,20 @@ static int uio_pdrv_genirq_probe(struct
> >> >> platform_device *pdev)
> >> >>
> >> >> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> >> >> dev_err(>dev, "missing platform_data\n");
> >> >> - goto bad0;
> >> >> + goto out_uioinfo;
> >> >> }
> >> >>
> >> >> if (uioinfo->handler || uioinfo->irqcontrol ||
> >> >> uioinfo->irq_flags & IRQF_SHARED) {
> >> >> dev_err(>dev, "interrupt configuration error\n");
> >> >> - goto bad0;
> >> >> + goto out_uioinfo;
> >> >> }
> >> >>
> >> >> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >> >> if (!priv) {
> >> >> ret = -ENOMEM;
> >> >> dev_err(>dev, "unable to kmalloc\n");
> >> >> - goto bad0;
> >> >> + goto out_uioinfo;
> >> >> }
> >> >>
> >> >> priv->uioinfo = uioinfo;
> >> >> @@ -150,7 +150,7 @@ static int uio_pdrv_genirq_probe(struct
> >> >> platform_device *pdev)
> >> >> ret = platform_get_irq(pdev, 0);
> >> >> if (ret < 0) {
> >> >> dev_err(>dev, "failed to get IRQ\n");
> >> >> - goto bad0;
> >> >> + goto out_priv;
> >> >> }
> >> >> uioinfo->irq = ret;
> >> >> }
> >> >> @@ -205,19 +205,20 @@ static int uio_pdrv_genirq_probe(struct
> >> >> platform_device *pdev)
> >> >> ret = uio_register_device(>dev, priv->uioinfo);
> >> >> if (ret) {
> >> >> dev_err(>dev, "unable to register uio device\n");
> >> >> - goto bad1;
> >> >> + goto out_pm;
> >> >> }
> >> >>
> >> >> platform_set_drvdata(pdev, priv);
> >> >> return 0;
> >> >> - bad1:
> >> >> - kfree(priv);
> >> >> +out_pm:
> >> >> pm_runtime_disable(>dev);
> >> >&g
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Wed, Nov 28, 2012 at 10:58:32AM +0200, Vitalii Demianets wrote:
> On Wednesday 28 November 2012 00:43:41 Hans J. Koch wrote:
> >
> > Thanks, good catch, but why don't you simply do this:
> >
>
> Just a matter of personal preference.
Your patch: 1 files changed, 12 insertions(+), 4 deletions(-)
My patch: 1 files changed, 1 insertions(+), 1 deletions(-)
Both achieve exactly the same. That's not a matter of personal
preference, that's the difference between a working solution and
a good solution. In the kernel, we want the latter.
> As a maintainer you can apply either
> patch you want. I guess you would prefer your approach and I have no
> objections to that :)
That's not the right kind of comment. Don't make it a habit.
Thanks,
Hans
>
> >
> > >From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
> > From: "Hans J. Koch"
> > Date: Tue, 27 Nov 2012 23:38:00 +0100
> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> >
> > In two cases, the return value variable "ret" can be undefined.
> >
> > Signed-off-by: Hans J. Koch
> > ---
> > drivers/uio/uio.c |2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > index 5110f36..fc60e35 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -263,7 +263,7 @@ static struct class uio_class = {
> > */
> > static int uio_dev_add_attributes(struct uio_device *idev)
> > {
> > - int ret;
> > + int ret = -ENOMEM;
> > int mi, pi;
> > int map_found = 0;
> > int portio_found = 0;
>
>
>
> --
> With Best Regards,
> Vitalii Demianets
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Wed, Nov 28, 2012 at 10:58:32AM +0200, Vitalii Demianets wrote:
On Wednesday 28 November 2012 00:43:41 Hans J. Koch wrote:
Thanks, good catch, but why don't you simply do this:
Just a matter of personal preference.
Your patch: 1 files changed, 12 insertions(+), 4 deletions(-)
My patch: 1 files changed, 1 insertions(+), 1 deletions(-)
Both achieve exactly the same. That's not a matter of personal
preference, that's the difference between a working solution and
a good solution. In the kernel, we want the latter.
As a maintainer you can apply either
patch you want. I guess you would prefer your approach and I have no
objections to that :)
That's not the right kind of comment. Don't make it a habit.
Thanks,
Hans
From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
From: Hans J. Koch h...@hansjkoch.de
Date: Tue, 27 Nov 2012 23:38:00 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable ret can be undefined.
Signed-off-by: Hans J. Koch h...@hansjkoch.de
---
drivers/uio/uio.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..fc60e35 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -263,7 +263,7 @@ static struct class uio_class = {
*/
static int uio_dev_add_attributes(struct uio_device *idev)
{
- int ret;
+ int ret = -ENOMEM;
int mi, pi;
int map_found = 0;
int portio_found = 0;
--
With Best Regards,
Vitalii Demianets
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak confusing labels
On Wed, Nov 28, 2012 at 10:29:29AM +0100, Cong Ding wrote:
On Wed, Nov 28, 2012 at 1:37 AM, Hans J. Koch h...@hansjkoch.de wrote:
On Wed, Nov 28, 2012 at 01:07:26AM +0100, Cong Ding wrote:
On Wed, Nov 28, 2012 at 12:07 AM, Hans J. Koch h...@hansjkoch.de wrote:
On Tue, Nov 27, 2012 at 07:29:32PM +0200, Vitalii Demianets wrote:
Memory leak was caused by jumping to the wrong exit label. So, it is
good time
to improve misleading label names too.
I agree that bad0, bad1, and bad2 are not the best choice for label
names...
I don't have any objections to your renaming.
But there's a more serious bug, maybe you can fix that as well while
you're
at it? (See below)
Thanks,
Hans
Signed-off-by: Vitalii Demianets vi...@nppfactor.kiev.ua
---
drivers/uio/uio_pdrv_genirq.c | 21 +++--
1 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/uio/uio_pdrv_genirq.c
b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..b88cf7b 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -110,7 +110,7 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
if (!uioinfo) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad2;
+ goto out;
}
uioinfo-name = pdev-dev.of_node-name;
uioinfo-version = devicetree;
@@ -125,20 +125,20 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
if (!uioinfo || !uioinfo-name || !uioinfo-version) {
dev_err(pdev-dev, missing platform_data\n);
- goto bad0;
+ goto out_uioinfo;
}
if (uioinfo-handler || uioinfo-irqcontrol ||
uioinfo-irq_flags IRQF_SHARED) {
dev_err(pdev-dev, interrupt configuration error\n);
- goto bad0;
+ goto out_uioinfo;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad0;
+ goto out_uioinfo;
}
priv-uioinfo = uioinfo;
@@ -150,7 +150,7 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
ret = platform_get_irq(pdev, 0);
if (ret 0) {
dev_err(pdev-dev, failed to get IRQ\n);
- goto bad0;
+ goto out_priv;
}
uioinfo-irq = ret;
}
@@ -205,19 +205,20 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
ret = uio_register_device(pdev-dev, priv-uioinfo);
if (ret) {
dev_err(pdev-dev, unable to register uio device\n);
- goto bad1;
+ goto out_pm;
}
platform_set_drvdata(pdev, priv);
return 0;
- bad1:
- kfree(priv);
+out_pm:
pm_runtime_disable(pdev-dev);
- bad0:
+out_priv:
+ kfree(priv);
+out_uioinfo:
/* kfree uioinfo for OF */
if (pdev-dev.of_node)
kfree(uioinfo);
The free() depends on pdev-dev.of_node, while the allocation doesn't
That's another source of memory leaks.
I don't agree. In line 99, it is
struct uio_info *uioinfo = pdev-dev.platform_data;
if uioinfo doesn't equal to NULL, it will run to line 126,
if (!uioinfo || !uioinfo-name || !uioinfo-version) {
and then if uioinfo-name equals to NULL, it runs to line 127 and 128,
and then goto bad0. If in this flow, we have to check
pdev-dev.of_node before free(uioinfo), right?
Hmmm. The idea is that uioinfo==NULL means OF. In that case,
a struct uio_info is allocated and filled with the necessary values (name,
version, irq). It is assumed (without check...) that pdev-dev.of_node
is not NULL. If it were NULL we would crash here when dereferencing
pdev-dev.of_node-name, leaving a memory leak.
After bad0 it is also assumed that pdev-dev.of_node is an indicator for
OF or not OF.
In other words, the case of uioinfo AND pdev-dev.of_node both being NULL
is not handled properly and will have ugly results.
You are correct, we have to ensure they are valid before line 115 (or
109). Sorry for misunderstanding your idea in the former email.
btw, I think in line 126 it is not necessary to check (!uioinfo),
because if uioinfo equals to NULL, it will go to line 109, and if the
alloc fails, it will go to bad2. uioinfo has no chance to be NULL when
runs to line 126. So I'd like to suggest a patch to avoid unnecessary
check like this
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..3eb4fa2 100644
--- a/drivers/uio
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak confusing labels
On Wed, Nov 28, 2012 at 11:37:23AM +0200, Vitalii Demianets wrote: On Wednesday 28 November 2012 02:37:50 Hans J. Koch wrote: In other words, the case of uioinfo AND pdev-dev.of_node both being NULL is not handled properly and will have ugly results. Moreover, the case of (uioinfo != NULL) (pdev-dev.of_node != NULL) leads to equally ugly results too (freeing uoinfo when it is statically allocated). You're right. That wants to be fixed as well. I think, we should sort out these problems, but in another patch. It is totally unrelated to the problem solved by the original patch (memory leak caused by not freeing priv in case platform_get_irq() fails). So far, no patch was applied. I don't mind if all fixes for uio_pdrv_genirq are in one patch as long as only this one file is altered. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak & confusing labels
On Wed, Nov 28, 2012 at 01:07:26AM +0100, Cong Ding wrote:
> On Wed, Nov 28, 2012 at 12:07 AM, Hans J. Koch wrote:
> > On Tue, Nov 27, 2012 at 07:29:32PM +0200, Vitalii Demianets wrote:
> >> Memory leak was caused by jumping to the wrong exit label. So, it is good
> >> time
> >> to improve misleading label names too.
> >
> > I agree that bad0, bad1, and bad2 are not the best choice for label names...
> > I don't have any objections to your renaming.
> >
> > But there's a more serious bug, maybe you can fix that as well while you're
> > at it? (See below)
> >
> > Thanks,
> > Hans
> >
> >>
> >> Signed-off-by: Vitalii Demianets
> >> ---
> >> drivers/uio/uio_pdrv_genirq.c | 21 +++--
> >> 1 files changed, 11 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> >> index 42202cd..b88cf7b 100644
> >> --- a/drivers/uio/uio_pdrv_genirq.c
> >> +++ b/drivers/uio/uio_pdrv_genirq.c
> >> @@ -110,7 +110,7 @@ static int uio_pdrv_genirq_probe(struct
> >> platform_device *pdev)
> >> if (!uioinfo) {
> >> ret = -ENOMEM;
> >> dev_err(>dev, "unable to kmalloc\n");
> >> - goto bad2;
> >> + goto out;
> >> }
> >> uioinfo->name = pdev->dev.of_node->name;
> >> uioinfo->version = "devicetree";
> >> @@ -125,20 +125,20 @@ static int uio_pdrv_genirq_probe(struct
> >> platform_device *pdev)
> >>
> >> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> >> dev_err(>dev, "missing platform_data\n");
> >> - goto bad0;
> >> + goto out_uioinfo;
> >> }
> >>
> >> if (uioinfo->handler || uioinfo->irqcontrol ||
> >> uioinfo->irq_flags & IRQF_SHARED) {
> >> dev_err(>dev, "interrupt configuration error\n");
> >> - goto bad0;
> >> + goto out_uioinfo;
> >> }
> >>
> >> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >> if (!priv) {
> >> ret = -ENOMEM;
> >> dev_err(>dev, "unable to kmalloc\n");
> >> - goto bad0;
> >> + goto out_uioinfo;
> >> }
> >>
> >> priv->uioinfo = uioinfo;
> >> @@ -150,7 +150,7 @@ static int uio_pdrv_genirq_probe(struct
> >> platform_device *pdev)
> >> ret = platform_get_irq(pdev, 0);
> >> if (ret < 0) {
> >> dev_err(>dev, "failed to get IRQ\n");
> >> - goto bad0;
> >> + goto out_priv;
> >> }
> >> uioinfo->irq = ret;
> >> }
> >> @@ -205,19 +205,20 @@ static int uio_pdrv_genirq_probe(struct
> >> platform_device *pdev)
> >> ret = uio_register_device(>dev, priv->uioinfo);
> >> if (ret) {
> >> dev_err(>dev, "unable to register uio device\n");
> >> - goto bad1;
> >> + goto out_pm;
> >> }
> >>
> >> platform_set_drvdata(pdev, priv);
> >> return 0;
> >> - bad1:
> >> - kfree(priv);
> >> +out_pm:
> >> pm_runtime_disable(>dev);
> >> - bad0:
> >> +out_priv:
> >> + kfree(priv);
> >> +out_uioinfo:
> >> /* kfree uioinfo for OF */
> >> if (pdev->dev.of_node)
> >> kfree(uioinfo);
> >
> > The free() depends on pdev->dev.of_node, while the allocation doesn't
> > That's another source of memory leaks.
> I don't agree. In line 99, it is
> struct uio_info *uioinfo = pdev->dev.platform_data;
> if uioinfo doesn't equal to NULL, it will run to line 126,
> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> and then if uioinfo->name equals to NULL, it runs to line 127 and 128,
> and then goto bad0. If in this flow, we have to check
> pdev->dev.of_node before free(uioinfo), right?
Hmmm. The idea is that uioinfo==NULL means OF. In that case,
a struct uio_info is allocated and filled with the necessary values (name,
version, irq). It is assu
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak & confusing labels
On Tue, Nov 27, 2012 at 07:29:32PM +0200, Vitalii Demianets wrote:
> Memory leak was caused by jumping to the wrong exit label. So, it is good time
> to improve misleading label names too.
I agree that bad0, bad1, and bad2 are not the best choice for label names...
I don't have any objections to your renaming.
But there's a more serious bug, maybe you can fix that as well while you're
at it? (See below)
Thanks,
Hans
>
> Signed-off-by: Vitalii Demianets
> ---
> drivers/uio/uio_pdrv_genirq.c | 21 +++--
> 1 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 42202cd..b88cf7b 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -110,7 +110,7 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> if (!uioinfo) {
> ret = -ENOMEM;
> dev_err(>dev, "unable to kmalloc\n");
> - goto bad2;
> + goto out;
> }
> uioinfo->name = pdev->dev.of_node->name;
> uioinfo->version = "devicetree";
> @@ -125,20 +125,20 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
>
> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> dev_err(>dev, "missing platform_data\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> if (uioinfo->handler || uioinfo->irqcontrol ||
> uioinfo->irq_flags & IRQF_SHARED) {
> dev_err(>dev, "interrupt configuration error\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> if (!priv) {
> ret = -ENOMEM;
> dev_err(>dev, "unable to kmalloc\n");
> - goto bad0;
> + goto out_uioinfo;
> }
>
> priv->uioinfo = uioinfo;
> @@ -150,7 +150,7 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> ret = platform_get_irq(pdev, 0);
> if (ret < 0) {
> dev_err(>dev, "failed to get IRQ\n");
> - goto bad0;
> + goto out_priv;
> }
> uioinfo->irq = ret;
> }
> @@ -205,19 +205,20 @@ static int uio_pdrv_genirq_probe(struct platform_device
> *pdev)
> ret = uio_register_device(>dev, priv->uioinfo);
> if (ret) {
> dev_err(>dev, "unable to register uio device\n");
> - goto bad1;
> + goto out_pm;
> }
>
> platform_set_drvdata(pdev, priv);
> return 0;
> - bad1:
> - kfree(priv);
> +out_pm:
> pm_runtime_disable(>dev);
> - bad0:
> +out_priv:
> + kfree(priv);
> +out_uioinfo:
> /* kfree uioinfo for OF */
> if (pdev->dev.of_node)
> kfree(uioinfo);
The free() depends on pdev->dev.of_node, while the allocation doesn't
That's another source of memory leaks.
> - bad2:
> +out:
> return ret;
> }
>
> --
> 1.7.8.6
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Tue, Nov 27, 2012 at 01:48:14PM +0200, Vitalii Demianets wrote:
> Fix warning: 'ret' might be used uninitialized
>
> Signed-off-by: Vitalii Demianets
> ---
> drivers/uio/uio.c | 16
> 1 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..c33fd18 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -280,12 +280,16 @@ static int uio_dev_add_attributes(struct uio_device
> *idev)
> map_found = 1;
> idev->map_dir = kobject_create_and_add("maps",
> >dev->kobj);
> - if (!idev->map_dir)
> + if (!idev->map_dir) {
> + ret = -ENOMEM;
> goto err_map;
> + }
> }
> map = kzalloc(sizeof(*map), GFP_KERNEL);
> - if (!map)
> + if (!map) {
> + ret = -ENOMEM;
> goto err_map;
> + }
> kobject_init(>kobj, _attr_type);
> map->mem = mem;
> mem->map = map;
> @@ -305,12 +309,16 @@ static int uio_dev_add_attributes(struct uio_device
> *idev)
> portio_found = 1;
> idev->portio_dir = kobject_create_and_add("portio",
> >dev->kobj);
> - if (!idev->portio_dir)
> + if (!idev->portio_dir) {
> + ret = -ENOMEM;
> goto err_portio;
> + }
> }
> portio = kzalloc(sizeof(*portio), GFP_KERNEL);
> - if (!portio)
> + if (!portio) {
> + ret = -ENOMEM;
> goto err_portio;
> + }
> kobject_init(>kobj, _attr_type);
> portio->port = port;
> port->portio = portio;
> --
> 1.7.8.6
>
Thanks, good catch, but why don't you simply do this:
>From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
From: "Hans J. Koch"
Date: Tue, 27 Nov 2012 23:38:00 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable "ret" can be undefined.
Signed-off-by: Hans J. Koch
---
drivers/uio/uio.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..fc60e35 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -263,7 +263,7 @@ static struct class uio_class = {
*/
static int uio_dev_add_attributes(struct uio_device *idev)
{
- int ret;
+ int ret = -ENOMEM;
int mi, pi;
int map_found = 0;
int portio_found = 0;
--
1.7.9
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
On Tue, Nov 27, 2012 at 01:48:14PM +0200, Vitalii Demianets wrote:
Fix warning: 'ret' might be used uninitialized
Signed-off-by: Vitalii Demianets vi...@nppfactor.kiev.ua
---
drivers/uio/uio.c | 16
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..c33fd18 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -280,12 +280,16 @@ static int uio_dev_add_attributes(struct uio_device
*idev)
map_found = 1;
idev-map_dir = kobject_create_and_add(maps,
idev-dev-kobj);
- if (!idev-map_dir)
+ if (!idev-map_dir) {
+ ret = -ENOMEM;
goto err_map;
+ }
}
map = kzalloc(sizeof(*map), GFP_KERNEL);
- if (!map)
+ if (!map) {
+ ret = -ENOMEM;
goto err_map;
+ }
kobject_init(map-kobj, map_attr_type);
map-mem = mem;
mem-map = map;
@@ -305,12 +309,16 @@ static int uio_dev_add_attributes(struct uio_device
*idev)
portio_found = 1;
idev-portio_dir = kobject_create_and_add(portio,
idev-dev-kobj);
- if (!idev-portio_dir)
+ if (!idev-portio_dir) {
+ ret = -ENOMEM;
goto err_portio;
+ }
}
portio = kzalloc(sizeof(*portio), GFP_KERNEL);
- if (!portio)
+ if (!portio) {
+ ret = -ENOMEM;
goto err_portio;
+ }
kobject_init(portio-kobj, portio_attr_type);
portio-port = port;
port-portio = portio;
--
1.7.8.6
Thanks, good catch, but why don't you simply do this:
From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
From: Hans J. Koch h...@hansjkoch.de
Date: Tue, 27 Nov 2012 23:38:00 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
In two cases, the return value variable ret can be undefined.
Signed-off-by: Hans J. Koch h...@hansjkoch.de
---
drivers/uio/uio.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..fc60e35 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -263,7 +263,7 @@ static struct class uio_class = {
*/
static int uio_dev_add_attributes(struct uio_device *idev)
{
- int ret;
+ int ret = -ENOMEM;
int mi, pi;
int map_found = 0;
int portio_found = 0;
--
1.7.9
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak confusing labels
On Tue, Nov 27, 2012 at 07:29:32PM +0200, Vitalii Demianets wrote:
Memory leak was caused by jumping to the wrong exit label. So, it is good time
to improve misleading label names too.
I agree that bad0, bad1, and bad2 are not the best choice for label names...
I don't have any objections to your renaming.
But there's a more serious bug, maybe you can fix that as well while you're
at it? (See below)
Thanks,
Hans
Signed-off-by: Vitalii Demianets vi...@nppfactor.kiev.ua
---
drivers/uio/uio_pdrv_genirq.c | 21 +++--
1 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..b88cf7b 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -110,7 +110,7 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
if (!uioinfo) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad2;
+ goto out;
}
uioinfo-name = pdev-dev.of_node-name;
uioinfo-version = devicetree;
@@ -125,20 +125,20 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
if (!uioinfo || !uioinfo-name || !uioinfo-version) {
dev_err(pdev-dev, missing platform_data\n);
- goto bad0;
+ goto out_uioinfo;
}
if (uioinfo-handler || uioinfo-irqcontrol ||
uioinfo-irq_flags IRQF_SHARED) {
dev_err(pdev-dev, interrupt configuration error\n);
- goto bad0;
+ goto out_uioinfo;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad0;
+ goto out_uioinfo;
}
priv-uioinfo = uioinfo;
@@ -150,7 +150,7 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
ret = platform_get_irq(pdev, 0);
if (ret 0) {
dev_err(pdev-dev, failed to get IRQ\n);
- goto bad0;
+ goto out_priv;
}
uioinfo-irq = ret;
}
@@ -205,19 +205,20 @@ static int uio_pdrv_genirq_probe(struct platform_device
*pdev)
ret = uio_register_device(pdev-dev, priv-uioinfo);
if (ret) {
dev_err(pdev-dev, unable to register uio device\n);
- goto bad1;
+ goto out_pm;
}
platform_set_drvdata(pdev, priv);
return 0;
- bad1:
- kfree(priv);
+out_pm:
pm_runtime_disable(pdev-dev);
- bad0:
+out_priv:
+ kfree(priv);
+out_uioinfo:
/* kfree uioinfo for OF */
if (pdev-dev.of_node)
kfree(uioinfo);
The free() depends on pdev-dev.of_node, while the allocation doesn't
That's another source of memory leaks.
- bad2:
+out:
return ret;
}
--
1.7.8.6
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/uio/uio_pdrv_genirq.c: Fix memory leak confusing labels
On Wed, Nov 28, 2012 at 01:07:26AM +0100, Cong Ding wrote:
On Wed, Nov 28, 2012 at 12:07 AM, Hans J. Koch h...@hansjkoch.de wrote:
On Tue, Nov 27, 2012 at 07:29:32PM +0200, Vitalii Demianets wrote:
Memory leak was caused by jumping to the wrong exit label. So, it is good
time
to improve misleading label names too.
I agree that bad0, bad1, and bad2 are not the best choice for label names...
I don't have any objections to your renaming.
But there's a more serious bug, maybe you can fix that as well while you're
at it? (See below)
Thanks,
Hans
Signed-off-by: Vitalii Demianets vi...@nppfactor.kiev.ua
---
drivers/uio/uio_pdrv_genirq.c | 21 +++--
1 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..b88cf7b 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -110,7 +110,7 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
if (!uioinfo) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad2;
+ goto out;
}
uioinfo-name = pdev-dev.of_node-name;
uioinfo-version = devicetree;
@@ -125,20 +125,20 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
if (!uioinfo || !uioinfo-name || !uioinfo-version) {
dev_err(pdev-dev, missing platform_data\n);
- goto bad0;
+ goto out_uioinfo;
}
if (uioinfo-handler || uioinfo-irqcontrol ||
uioinfo-irq_flags IRQF_SHARED) {
dev_err(pdev-dev, interrupt configuration error\n);
- goto bad0;
+ goto out_uioinfo;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv) {
ret = -ENOMEM;
dev_err(pdev-dev, unable to kmalloc\n);
- goto bad0;
+ goto out_uioinfo;
}
priv-uioinfo = uioinfo;
@@ -150,7 +150,7 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
ret = platform_get_irq(pdev, 0);
if (ret 0) {
dev_err(pdev-dev, failed to get IRQ\n);
- goto bad0;
+ goto out_priv;
}
uioinfo-irq = ret;
}
@@ -205,19 +205,20 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
ret = uio_register_device(pdev-dev, priv-uioinfo);
if (ret) {
dev_err(pdev-dev, unable to register uio device\n);
- goto bad1;
+ goto out_pm;
}
platform_set_drvdata(pdev, priv);
return 0;
- bad1:
- kfree(priv);
+out_pm:
pm_runtime_disable(pdev-dev);
- bad0:
+out_priv:
+ kfree(priv);
+out_uioinfo:
/* kfree uioinfo for OF */
if (pdev-dev.of_node)
kfree(uioinfo);
The free() depends on pdev-dev.of_node, while the allocation doesn't
That's another source of memory leaks.
I don't agree. In line 99, it is
struct uio_info *uioinfo = pdev-dev.platform_data;
if uioinfo doesn't equal to NULL, it will run to line 126,
if (!uioinfo || !uioinfo-name || !uioinfo-version) {
and then if uioinfo-name equals to NULL, it runs to line 127 and 128,
and then goto bad0. If in this flow, we have to check
pdev-dev.of_node before free(uioinfo), right?
Hmmm. The idea is that uioinfo==NULL means OF. In that case,
a struct uio_info is allocated and filled with the necessary values (name,
version, irq). It is assumed (without check...) that pdev-dev.of_node
is not NULL. If it were NULL we would crash here when dereferencing
pdev-dev.of_node-name, leaving a memory leak.
After bad0 it is also assumed that pdev-dev.of_node is an indicator for
OF or not OF.
In other words, the case of uioinfo AND pdev-dev.of_node both being NULL
is not handled properly and will have ugly results.
btw, I think in line 126 it is not necessary to check (!uioinfo),
because if uioinfo equals to NULL, it will go to line 109, and if the
alloc fails, it will go to bad2. uioinfo has no chance to be NULL when
runs to line 126. So I'd like to suggest a patch to avoid unnecessary
check like this
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..3eb4fa2 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -123,7 +123,7 @@ static int uio_pdrv_genirq_probe(struct
platform_device *pdev)
uioinfo-irq = irq;
}
- if (!uioinfo || !uioinfo-name || !uioinfo-version) {
+ if (!uioinfo-name || !uioinfo-version) {
That's wrong. We need a valid uioinfo at this point.
dev_err(pdev-dev, missing platform_data\n);
goto bad0
Re: [PATCH] uio_pdrv: set memory mapping name
On Fri, Nov 09, 2012 at 07:06:40AM +0100, Manuel Traut wrote: > If uio_pdrv[_genirq] is used, the uio maps have currently no name set. > This patch sets the uio_mem name to the name of the memory resource. Looks fine to me. I added the driver's authors to Cc. Signed-off-by: "Hans J. Koch" > > Signed-off-by: Manuel Traut > Reported-by: Stefan Staedtler > Tested-by: Stefan Staedtler > > diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c > index 42202cd..ac988ce 100644 > --- a/drivers/uio/uio_pdrv_genirq.c > +++ b/drivers/uio/uio_pdrv_genirq.c > @@ -172,6 +172,7 @@ static int uio_pdrv_genirq_probe(struct platform_device > *pdev) > uiomem->memtype = UIO_MEM_PHYS; > uiomem->addr = r->start; > uiomem->size = resource_size(r); > + uiomem->name = r->name; > ++uiomem; > } > > diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c > index 72d3646..39be9e0 100644 > --- a/drivers/uio/uio_pdrv.c > +++ b/drivers/uio/uio_pdrv.c > @@ -60,6 +60,7 @@ static int uio_pdrv_probe(struct platform_device *pdev) > uiomem->memtype = UIO_MEM_PHYS; > uiomem->addr = r->start; > uiomem->size = resource_size(r); > + uiomem->name = r->name; > ++uiomem; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uio_pdrv: set memory mapping name
On Fri, Nov 09, 2012 at 07:06:40AM +0100, Manuel Traut wrote: If uio_pdrv[_genirq] is used, the uio maps have currently no name set. This patch sets the uio_mem name to the name of the memory resource. Looks fine to me. I added the driver's authors to Cc. Signed-off-by: Hans J. Koch h...@hansjkoch.de Signed-off-by: Manuel Traut ma...@linutronix.de Reported-by: Stefan Staedtler stefan.staedt...@siemens.com Tested-by: Stefan Staedtler stefan.staedt...@siemens.com diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c index 42202cd..ac988ce 100644 --- a/drivers/uio/uio_pdrv_genirq.c +++ b/drivers/uio/uio_pdrv_genirq.c @@ -172,6 +172,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev) uiomem-memtype = UIO_MEM_PHYS; uiomem-addr = r-start; uiomem-size = resource_size(r); + uiomem-name = r-name; ++uiomem; } diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c index 72d3646..39be9e0 100644 --- a/drivers/uio/uio_pdrv.c +++ b/drivers/uio/uio_pdrv.c @@ -60,6 +60,7 @@ static int uio_pdrv_probe(struct platform_device *pdev) uiomem-memtype = UIO_MEM_PHYS; uiomem-addr = r-start; uiomem-size = resource_size(r); + uiomem-name = r-name; ++uiomem; } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] uio: uio_pruss: replace private SRAM API with genalloc
On Thu, Oct 18, 2012 at 10:53:38AM -0400, Matt Porter wrote:
> On Fri, Oct 05, 2012 at 01:04:40PM -0400, Matt Porter wrote:
> > Remove the use of the private DaVinci SRAM API in favor
> > of genalloc. The pool to be used is provided by platform
> > data.
> >
> > Signed-off-by: Matt Porter
>
> Hans,
>
> Any additional concerns on this patch?
Not from my side. I was waiting for your discussion to calm down.
> Sekhar is holding off on applying
> parts 4,5,7 for davinci until this is accepted.
You can add my
Signed-off-by: "Hans J. Koch"
Thanks,
Hans
>
> Thanks,
> Matt
>
> > ---
> > drivers/uio/Kconfig |1 +
> > drivers/uio/uio_pruss.c | 24 +---
> > include/linux/platform_data/uio_pruss.h |3 ++-
> > 3 files changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> > index 6f3ea9b..c48b938 100644
> > --- a/drivers/uio/Kconfig
> > +++ b/drivers/uio/Kconfig
> > @@ -97,6 +97,7 @@ config UIO_NETX
> > config UIO_PRUSS
> > tristate "Texas Instruments PRUSS driver"
> > depends on ARCH_DAVINCI_DA850
> > + select GENERIC_ALLOCATOR
> > help
> > PRUSS driver for OMAPL138/DA850/AM18XX devices
> > PRUSS driver requires user space components, examples and user space
> > diff --git a/drivers/uio/uio_pruss.c b/drivers/uio/uio_pruss.c
> > index 33a7a27..f8738de 100644
> > --- a/drivers/uio/uio_pruss.c
> > +++ b/drivers/uio/uio_pruss.c
> > @@ -25,7 +25,7 @@
> > #include
> > #include
> > #include
> > -#include
> > +#include
> >
> > #define DRV_NAME "pruss_uio"
> > #define DRV_VERSION "1.0"
> > @@ -65,10 +65,11 @@ struct uio_pruss_dev {
> > dma_addr_t sram_paddr;
> > dma_addr_t ddr_paddr;
> > void __iomem *prussio_vaddr;
> > - void *sram_vaddr;
> > + unsigned long sram_vaddr;
> > void *ddr_vaddr;
> > unsigned int hostirq_start;
> > unsigned int pintc_base;
> > + struct gen_pool *sram_pool;
> > };
> >
> > static irqreturn_t pruss_handler(int irq, struct uio_info *info)
> > @@ -106,7 +107,9 @@ static void pruss_cleanup(struct platform_device *dev,
> > gdev->ddr_paddr);
> > }
> > if (gdev->sram_vaddr)
> > - sram_free(gdev->sram_vaddr, sram_pool_sz);
> > + gen_pool_free(gdev->sram_pool,
> > + gdev->sram_vaddr,
> > + sram_pool_sz);
> > kfree(gdev->info);
> > clk_put(gdev->pruss_clk);
> > kfree(gdev);
> > @@ -152,10 +155,17 @@ static int __devinit pruss_probe(struct
> > platform_device *dev)
> > goto out_free;
> > }
> >
> > - gdev->sram_vaddr = sram_alloc(sram_pool_sz, &(gdev->sram_paddr));
> > - if (!gdev->sram_vaddr) {
> > - dev_err(>dev, "Could not allocate SRAM pool\n");
> > - goto out_free;
> > + if (pdata->sram_pool) {
> > + gdev->sram_pool = pdata->sram_pool;
> > + gdev->sram_vaddr =
> > + gen_pool_alloc(gdev->sram_pool, sram_pool_sz);
> > + if (!gdev->sram_vaddr) {
> > + dev_err(>dev, "Could not allocate SRAM pool\n");
> > + goto out_free;
> > + }
> > + gdev->sram_paddr =
> > + gen_pool_virt_to_phys(gdev->sram_pool,
> > + gdev->sram_vaddr);
> > }
> >
> > gdev->ddr_vaddr = dma_alloc_coherent(>dev, extram_pool_sz,
> > diff --git a/include/linux/platform_data/uio_pruss.h
> > b/include/linux/platform_data/uio_pruss.h
> > index f39140a..3d47d21 100644
> > --- a/include/linux/platform_data/uio_pruss.h
> > +++ b/include/linux/platform_data/uio_pruss.h
> > @@ -20,6 +20,7 @@
> >
> > /* To configure the PRUSS INTC base offset for UIO driver */
> > struct uio_pruss_pdata {
> > - u32 pintc_base;
> > + u32 pintc_base;
> > + struct gen_pool *sram_pool;
> > };
> > #endif /* _UIO_PRUSS_H_ */
> > --
> > 1.7.9.5
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/7] uio: uio_pruss: replace private SRAM API with genalloc
On Thu, Oct 18, 2012 at 10:53:38AM -0400, Matt Porter wrote:
On Fri, Oct 05, 2012 at 01:04:40PM -0400, Matt Porter wrote:
Remove the use of the private DaVinci SRAM API in favor
of genalloc. The pool to be used is provided by platform
data.
Signed-off-by: Matt Porter mpor...@ti.com
Hans,
Any additional concerns on this patch?
Not from my side. I was waiting for your discussion to calm down.
Sekhar is holding off on applying
parts 4,5,7 for davinci until this is accepted.
You can add my
Signed-off-by: Hans J. Koch h...@hansjkoch.de
Thanks,
Hans
Thanks,
Matt
---
drivers/uio/Kconfig |1 +
drivers/uio/uio_pruss.c | 24 +---
include/linux/platform_data/uio_pruss.h |3 ++-
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 6f3ea9b..c48b938 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -97,6 +97,7 @@ config UIO_NETX
config UIO_PRUSS
tristate Texas Instruments PRUSS driver
depends on ARCH_DAVINCI_DA850
+ select GENERIC_ALLOCATOR
help
PRUSS driver for OMAPL138/DA850/AM18XX devices
PRUSS driver requires user space components, examples and user space
diff --git a/drivers/uio/uio_pruss.c b/drivers/uio/uio_pruss.c
index 33a7a27..f8738de 100644
--- a/drivers/uio/uio_pruss.c
+++ b/drivers/uio/uio_pruss.c
@@ -25,7 +25,7 @@
#include linux/clk.h
#include linux/dma-mapping.h
#include linux/slab.h
-#include mach/sram.h
+#include linux/genalloc.h
#define DRV_NAME pruss_uio
#define DRV_VERSION 1.0
@@ -65,10 +65,11 @@ struct uio_pruss_dev {
dma_addr_t sram_paddr;
dma_addr_t ddr_paddr;
void __iomem *prussio_vaddr;
- void *sram_vaddr;
+ unsigned long sram_vaddr;
void *ddr_vaddr;
unsigned int hostirq_start;
unsigned int pintc_base;
+ struct gen_pool *sram_pool;
};
static irqreturn_t pruss_handler(int irq, struct uio_info *info)
@@ -106,7 +107,9 @@ static void pruss_cleanup(struct platform_device *dev,
gdev-ddr_paddr);
}
if (gdev-sram_vaddr)
- sram_free(gdev-sram_vaddr, sram_pool_sz);
+ gen_pool_free(gdev-sram_pool,
+ gdev-sram_vaddr,
+ sram_pool_sz);
kfree(gdev-info);
clk_put(gdev-pruss_clk);
kfree(gdev);
@@ -152,10 +155,17 @@ static int __devinit pruss_probe(struct
platform_device *dev)
goto out_free;
}
- gdev-sram_vaddr = sram_alloc(sram_pool_sz, (gdev-sram_paddr));
- if (!gdev-sram_vaddr) {
- dev_err(dev-dev, Could not allocate SRAM pool\n);
- goto out_free;
+ if (pdata-sram_pool) {
+ gdev-sram_pool = pdata-sram_pool;
+ gdev-sram_vaddr =
+ gen_pool_alloc(gdev-sram_pool, sram_pool_sz);
+ if (!gdev-sram_vaddr) {
+ dev_err(dev-dev, Could not allocate SRAM pool\n);
+ goto out_free;
+ }
+ gdev-sram_paddr =
+ gen_pool_virt_to_phys(gdev-sram_pool,
+ gdev-sram_vaddr);
}
gdev-ddr_vaddr = dma_alloc_coherent(dev-dev, extram_pool_sz,
diff --git a/include/linux/platform_data/uio_pruss.h
b/include/linux/platform_data/uio_pruss.h
index f39140a..3d47d21 100644
--- a/include/linux/platform_data/uio_pruss.h
+++ b/include/linux/platform_data/uio_pruss.h
@@ -20,6 +20,7 @@
/* To configure the PRUSS INTC base offset for UIO driver */
struct uio_pruss_pdata {
- u32 pintc_base;
+ u32 pintc_base;
+ struct gen_pool *sram_pool;
};
#endif /* _UIO_PRUSS_H_ */
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/3] uio: uio_pruss: port to AM33xx
On Wed, Sep 26, 2012 at 09:44:29AM -0400, Matt Porter wrote:
> Add ifdefery hacks to only use SRAM on Davinci. This
> needs to be cleaned up with a sane generic SRAM allocator
> (like the DT based driver available that can't be used on
> Davinci which is just starting DT conversion) before it
> can go upstream.
I agree with that ;-)
Needing lots of ifdefs in *.c files is usually a bad sign...
Thanks anyway for posting, it's good to have things like that in the archives.
And please make driver code and documentation updates separate patches in the
future.
Hans
>
> Adds DT, pinctrl, and runtime PM support for use on
> AM33xx.
>
> Signed-off-by: Matt Porter
> ---
> Documentation/devicetree/bindings/uio/pruss.txt| 17 ++
> .../devicetree/bindings/uio/uio_pruss.txt | 17 ++
> drivers/uio/Kconfig|4 +-
> drivers/uio/uio_pruss.c| 63
> +++-
> 4 files changed, 98 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/uio/pruss.txt
> create mode 100644 Documentation/devicetree/bindings/uio/uio_pruss.txt
>
> diff --git a/Documentation/devicetree/bindings/uio/pruss.txt
> b/Documentation/devicetree/bindings/uio/pruss.txt
> new file mode 100644
> index 000..2ac45c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/uio/pruss.txt
> @@ -0,0 +1,17 @@
> +TI PRUSS device
> +
> +Required properties:
> +- compatible :
> + - "ti,pruss-v1" for AM18xx/OMAP-L138/DA850
> + - "ti,pruss-v2" for AM33xx.
> +- ti,pintc-offset : Offset of the PINTC from the PRUSS address base
> +- ti,hwmods: Name of the hwmod associated to the PRUSS
> +
> +Example:
> +
> +pruss: pruss@4a30 {
> + compatible = "ti,pruss-v2";
> + ti,hwmods = "pruss";
> + reg = <0x4a30 0x08>;
> + ti,pintc-offset = <0x2>;
> +};
> diff --git a/Documentation/devicetree/bindings/uio/uio_pruss.txt
> b/Documentation/devicetree/bindings/uio/uio_pruss.txt
> new file mode 100644
> index 000..2ac45c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/uio/uio_pruss.txt
> @@ -0,0 +1,17 @@
> +TI PRUSS device
> +
> +Required properties:
> +- compatible :
> + - "ti,pruss-v1" for AM18xx/OMAP-L138/DA850
> + - "ti,pruss-v2" for AM33xx.
> +- ti,pintc-offset : Offset of the PINTC from the PRUSS address base
> +- ti,hwmods: Name of the hwmod associated to the PRUSS
> +
> +Example:
> +
> +pruss: pruss@4a30 {
> + compatible = "ti,pruss-v2";
> + ti,hwmods = "pruss";
> + reg = <0x4a30 0x08>;
> + ti,pintc-offset = <0x2>;
> +};
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 6f3ea9b..8da7d9b 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -96,9 +96,9 @@ config UIO_NETX
>
> config UIO_PRUSS
> tristate "Texas Instruments PRUSS driver"
> - depends on ARCH_DAVINCI_DA850
> + depends on ARCH_DAVINCI_DA850 || SOC_AM33XX
> help
> - PRUSS driver for OMAPL138/DA850/AM18XX devices
> + PRUSS driver for OMAPL138/DA850/AM18XX and AM33XX devices
> PRUSS driver requires user space components, examples and user space
> driver is available from below SVN repo - you may use anonymous login
>
> diff --git a/drivers/uio/uio_pruss.c b/drivers/uio/uio_pruss.c
> index 33a7a27..326ce40 100644
> --- a/drivers/uio/uio_pruss.c
> +++ b/drivers/uio/uio_pruss.c
> @@ -25,7 +25,15 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#ifdef CONFIG_ARCH_DAVINCI_DA850
> #include
> +#endif
>
> #define DRV_NAME "pruss_uio"
> #define DRV_VERSION "1.0"
> @@ -105,8 +113,10 @@ static void pruss_cleanup(struct platform_device *dev,
> dma_free_coherent(>dev, extram_pool_sz, gdev->ddr_vaddr,
> gdev->ddr_paddr);
> }
> +#ifdef CONFIG_ARCH_DAVINCI_DA850
> if (gdev->sram_vaddr)
> sram_free(gdev->sram_vaddr, sram_pool_sz);
> +#endif
> kfree(gdev->info);
> clk_put(gdev->pruss_clk);
> kfree(gdev);
> @@ -117,8 +127,10 @@ static int __devinit pruss_probe(struct platform_device
> *dev)
> struct uio_info *p;
> struct uio_pruss_dev *gdev;
> struct resource *regs_prussio;
> + struct resource res;
> int ret = -ENODEV, cnt = 0, len;
> struct uio_pruss_pdata *pdata = dev->dev.platform_data;
> + struct pinctrl *pinctrl;
>
> gdev = kzalloc(sizeof(struct uio_pruss_dev), GFP_KERNEL);
> if (!gdev)
> @@ -129,6 +141,7 @@ static int __devinit pruss_probe(struct platform_device
> *dev)
> kfree(gdev);
> return -ENOMEM;
> }
> +#ifdef CONFIG_ARCH_DAVINCI_DA850
> /* Power on PRU in case its not done as part of boot-loader */
> gdev->pruss_clk = clk_get(>dev, "pruss");
> if (IS_ERR(gdev->pruss_clk)) {
> @@ -140,6 +153,28 @@ static int __devinit pruss_probe(struct platform_device
> *dev)
> }
Re: [v2 PATCH 2/2] Add uio_dmem_genirq description to UIO documentation
On Tue, Sep 25, 2012 at 03:09:12PM +0900, Damian Hobson-Garcia wrote: > Signed-off-by: Damian Hobson-Garcia Signed-off-by: "Hans J. Koch" > --- > Documentation/DocBook/uio-howto.tmpl | 56 > ++ > 1 files changed, 56 insertions(+), 0 deletions(-) > > diff --git a/Documentation/DocBook/uio-howto.tmpl > b/Documentation/DocBook/uio-howto.tmpl > index ac3d001..db08c1a 100644 > --- a/Documentation/DocBook/uio-howto.tmpl > +++ b/Documentation/DocBook/uio-howto.tmpl > @@ -719,6 +719,62 @@ framework to set up sysfs files for this region. Simply > leave it alone. > > > > + > +Using uio_dmem_genirq for platform devices > + > + In addition to statically allocated memory ranges, they may also be > + a desire to use dynamically allocated regions in a user space driver. > + In particular, being able to access memory made available through the > + dma-mapping API, may be particularly useful. The > + uio_dmem_genirq driver provides a way to accomplish > + this. > + > + > + This driver is used in a similar manner to the > + "uio_pdrv_genirq" driver with respect to interrupt > + configuration and handling. > + > + > + Set the .name element of > + struct platform_device to > + "uio_dmem_genirq" to use this driver. > + > + > + When using this driver, fill in the .platform_data > + element of struct platform_device, which is of type > + struct uio_dmem_genirq_pdata and which contains the > + following elements: > + > + > + struct uio_info uioinfo: The same > + structure used as the uio_pdrv_genirq platform > + data > + unsigned int *dynamic_region_sizes: > + Pointer to list of sizes of dynamic memory regions to be mapped into > + user space. > + > + unsigned int num_dynamic_regions: > + Number of elements in dynamic_region_sizes array. > + > + > + > + The dynamic regions defined in the platform data will be appended to > + the mem[] array after the platform device > + resources, which implies that the total number of static and dynamic > + memory regions cannot exceed MAX_UIO_MAPS. > + > + > + The dynamic memory regions will be allocated when the UIO device file, > + /dev/uioX is opened. > + Simiar to static memory resources, the memory region information for > + dynamic regions is then visible via sysfs at > + /sys/class/uio/uioX/maps/mapY/*. > + The dynmaic memory regions will be freed when the UIO device file is > + closed. When no processes are holding the device file open, the address > + returned to userspace is DMA_ERROR_CODE. > + > + > + > > > > -- > 1.7.5.4 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [v2 PATCH 1/2] Add new uio device for dynamic memory allocation
On Tue, Sep 25, 2012 at 03:09:11PM +0900, Damian Hobson-Garcia wrote:
> This device extends the uio_pdrv_genirq driver to provide limited
> dynamic memory allocation for UIO devices. This allows UIO devices
> to use CMA and IOMMU allocated memory regions. This driver is based
> on the uio_pdrv_genirq driver and provides the same generic interrupt
> handling capabilities. Like uio_prdv_genirq,
> a fixed number of memory regions, defined in the platform device's
> .resources field are exported to userpace. This driver adds the ability
> to export additional regions whose number and size are known at boot time,
> but whose memory is not allocated until the uio device file is opened for
> the first time. When the device file is closed, the allocated memory block
> is freed. Physical (DMA) addresses for the dynamic regions are provided to
> the userspace via /sys/class/uio/uioX/maps/mapY/addr in the same way as
> static addresses are when the uio device file is open, when no processes
> are holding the device file open, the address returned to userspace is
> DMA_ERROR_CODE.
>
> Signed-off-by: Damian Hobson-Garcia
Thanks for your contribution!
Signed-off-by: "Hans J. Koch"
> ---
> drivers/uio/Kconfig | 16 ++
> drivers/uio/Makefile |1 +
> drivers/uio/uio_dmem_genirq.c | 354
> +
> include/linux/platform_data/uio_dmem_genirq.h | 26 ++
> 4 files changed, 397 insertions(+), 0 deletions(-)
> create mode 100644 drivers/uio/uio_dmem_genirq.c
> create mode 100644 include/linux/platform_data/uio_dmem_genirq.h
>
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 6f3ea9b..82e2b89 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -44,6 +44,22 @@ config UIO_PDRV_GENIRQ
>
> If you don't know what to do here, say N.
>
> +config UIO_DMEM_GENIRQ
> + tristate "Userspace platform driver with generic irq and dynamic memory"
> + help
> + Platform driver for Userspace I/O devices, including generic
> + interrupt handling code. Shared interrupts are not supported.
> +
> + Memory regions can be specified with the same platform device
> + resources as the UIO_PDRV drivers, but dynamic regions can also
> + be specified.
> + The number and size of these regions is static,
> + but the memory allocation is not performed until
> + the associated device file is opened. The
> + memory is freed once the uio device is closed.
> +
> + If you don't know what to do here, say N.
> +
> config UIO_AEC
> tristate "AEC video timestamp device"
> depends on PCI
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index d4dd9a5..b354c53 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_UIO) += uio.o
> obj-$(CONFIG_UIO_CIF)+= uio_cif.o
> obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
> obj-$(CONFIG_UIO_PDRV_GENIRQ)+= uio_pdrv_genirq.o
> +obj-$(CONFIG_UIO_DMEM_GENIRQ)+= uio_dmem_genirq.o
> obj-$(CONFIG_UIO_AEC)+= uio_aec.o
> obj-$(CONFIG_UIO_SERCOS3)+= uio_sercos3.o
> obj-$(CONFIG_UIO_PCI_GENERIC)+= uio_pci_generic.o
> diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
> new file mode 100644
> index 000..4d4dd00
> --- /dev/null
> +++ b/drivers/uio/uio_dmem_genirq.c
> @@ -0,0 +1,354 @@
> +/*
> + * drivers/uio/uio_dmem_genirq.c
> + *
> + * Userspace I/O platform driver with generic IRQ handling code.
> + *
> + * Copyright (C) 2012 Damian Hobson-Garcia
> + *
> + * Based on uio_pdrv_genirq.c by Magnus Damm
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "uio_dmem_genirq"
> +
> +struct uio_dmem_genirq_platdata {
> + struct uio_info *uioinfo;
> + spinlock_t lock;
> + unsigned long flags;
> + struct platform_device *pdev;
> + unsigned int dmem_region_start;
> + unsigned int num_dmem_regions;
> + struct mutex alloc_lock;
> + unsigned int refcnt;
> +};
> +
> +static int uio_dmem_genirq_open(struct uio_info *info, struct inode *inode)
> +{
> + struct uio_dmem_genirq_platdata *priv = in
Re: [v2 PATCH 1/2] Add new uio device for dynamic memory allocation
On Tue, Sep 25, 2012 at 03:09:11PM +0900, Damian Hobson-Garcia wrote:
This device extends the uio_pdrv_genirq driver to provide limited
dynamic memory allocation for UIO devices. This allows UIO devices
to use CMA and IOMMU allocated memory regions. This driver is based
on the uio_pdrv_genirq driver and provides the same generic interrupt
handling capabilities. Like uio_prdv_genirq,
a fixed number of memory regions, defined in the platform device's
.resources field are exported to userpace. This driver adds the ability
to export additional regions whose number and size are known at boot time,
but whose memory is not allocated until the uio device file is opened for
the first time. When the device file is closed, the allocated memory block
is freed. Physical (DMA) addresses for the dynamic regions are provided to
the userspace via /sys/class/uio/uioX/maps/mapY/addr in the same way as
static addresses are when the uio device file is open, when no processes
are holding the device file open, the address returned to userspace is
DMA_ERROR_CODE.
Signed-off-by: Damian Hobson-Garcia dhobs...@igel.co.jp
Thanks for your contribution!
Signed-off-by: Hans J. Koch h...@hansjkoch.de
---
drivers/uio/Kconfig | 16 ++
drivers/uio/Makefile |1 +
drivers/uio/uio_dmem_genirq.c | 354
+
include/linux/platform_data/uio_dmem_genirq.h | 26 ++
4 files changed, 397 insertions(+), 0 deletions(-)
create mode 100644 drivers/uio/uio_dmem_genirq.c
create mode 100644 include/linux/platform_data/uio_dmem_genirq.h
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 6f3ea9b..82e2b89 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -44,6 +44,22 @@ config UIO_PDRV_GENIRQ
If you don't know what to do here, say N.
+config UIO_DMEM_GENIRQ
+ tristate Userspace platform driver with generic irq and dynamic memory
+ help
+ Platform driver for Userspace I/O devices, including generic
+ interrupt handling code. Shared interrupts are not supported.
+
+ Memory regions can be specified with the same platform device
+ resources as the UIO_PDRV drivers, but dynamic regions can also
+ be specified.
+ The number and size of these regions is static,
+ but the memory allocation is not performed until
+ the associated device file is opened. The
+ memory is freed once the uio device is closed.
+
+ If you don't know what to do here, say N.
+
config UIO_AEC
tristate AEC video timestamp device
depends on PCI
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index d4dd9a5..b354c53 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_UIO) += uio.o
obj-$(CONFIG_UIO_CIF)+= uio_cif.o
obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
obj-$(CONFIG_UIO_PDRV_GENIRQ)+= uio_pdrv_genirq.o
+obj-$(CONFIG_UIO_DMEM_GENIRQ)+= uio_dmem_genirq.o
obj-$(CONFIG_UIO_AEC)+= uio_aec.o
obj-$(CONFIG_UIO_SERCOS3)+= uio_sercos3.o
obj-$(CONFIG_UIO_PCI_GENERIC)+= uio_pci_generic.o
diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
new file mode 100644
index 000..4d4dd00
--- /dev/null
+++ b/drivers/uio/uio_dmem_genirq.c
@@ -0,0 +1,354 @@
+/*
+ * drivers/uio/uio_dmem_genirq.c
+ *
+ * Userspace I/O platform driver with generic IRQ handling code.
+ *
+ * Copyright (C) 2012 Damian Hobson-Garcia
+ *
+ * Based on uio_pdrv_genirq.c by Magnus Damm
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
by
+ * the Free Software Foundation.
+ */
+
+#include linux/platform_device.h
+#include linux/uio_driver.h
+#include linux/spinlock.h
+#include linux/bitops.h
+#include linux/module.h
+#include linux/interrupt.h
+#include linux/platform_data/uio_dmem_genirq.h
+#include linux/stringify.h
+#include linux/pm_runtime.h
+#include linux/dma-mapping.h
+#include linux/slab.h
+
+#include linux/of.h
+#include linux/of_platform.h
+#include linux/of_address.h
+
+#define DRIVER_NAME uio_dmem_genirq
+
+struct uio_dmem_genirq_platdata {
+ struct uio_info *uioinfo;
+ spinlock_t lock;
+ unsigned long flags;
+ struct platform_device *pdev;
+ unsigned int dmem_region_start;
+ unsigned int num_dmem_regions;
+ struct mutex alloc_lock;
+ unsigned int refcnt;
+};
+
+static int uio_dmem_genirq_open(struct uio_info *info, struct inode *inode)
+{
+ struct uio_dmem_genirq_platdata *priv = info-priv;
+ struct uio_mem *uiomem;
+ int ret = 0;
+
+ uiomem = priv-uioinfo-mem[priv-dmem_region_start];
+
+ mutex_lock(priv-alloc_lock);
+ while (!priv-refcnt uiomem priv-uioinfo-mem[MAX_UIO_MAPS]) {
+ void
Re: [v2 PATCH 2/2] Add uio_dmem_genirq description to UIO documentation
On Tue, Sep 25, 2012 at 03:09:12PM +0900, Damian Hobson-Garcia wrote: Signed-off-by: Damian Hobson-Garcia dhobs...@igel.co.jp Signed-off-by: Hans J. Koch h...@hansjkoch.de --- Documentation/DocBook/uio-howto.tmpl | 56 ++ 1 files changed, 56 insertions(+), 0 deletions(-) diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl index ac3d001..db08c1a 100644 --- a/Documentation/DocBook/uio-howto.tmpl +++ b/Documentation/DocBook/uio-howto.tmpl @@ -719,6 +719,62 @@ framework to set up sysfs files for this region. Simply leave it alone. /para /sect1 +sect1 id=using uio_dmem_genirq +titleUsing uio_dmem_genirq for platform devices/title + para + In addition to statically allocated memory ranges, they may also be + a desire to use dynamically allocated regions in a user space driver. + In particular, being able to access memory made available through the + dma-mapping API, may be particularly useful. The + varnameuio_dmem_genirq/varname driver provides a way to accomplish + this. + /para + para + This driver is used in a similar manner to the + varnameuio_pdrv_genirq/varname driver with respect to interrupt + configuration and handling. + /para + para + Set the varname.name/varname element of + varnamestruct platform_device/varname to + varnameuio_dmem_genirq/varname to use this driver. + /para + para + When using this driver, fill in the varname.platform_data/varname + element of varnamestruct platform_device/varname, which is of type + varnamestruct uio_dmem_genirq_pdata/varname and which contains the + following elements: + /para + itemizedlist + listitemvarnamestruct uio_info uioinfo/varname: The same + structure used as the varnameuio_pdrv_genirq/varname platform + data/listitem + listitemvarnameunsigned int *dynamic_region_sizes/varname: + Pointer to list of sizes of dynamic memory regions to be mapped into + user space. + /listitem + listitemvarnameunsigned int num_dynamic_regions/varname: + Number of elements in varnamedynamic_region_sizes/varname array. + /listitem + /itemizedlist + para + The dynamic regions defined in the platform data will be appended to + the varname mem[] /varname array after the platform device + resources, which implies that the total number of static and dynamic + memory regions cannot exceed varnameMAX_UIO_MAPS/varname. + /para + para + The dynamic memory regions will be allocated when the UIO device file, + varname/dev/uioX/varname is opened. + Simiar to static memory resources, the memory region information for + dynamic regions is then visible via sysfs at + varname/sys/class/uio/uioX/maps/mapY/*/varname. + The dynmaic memory regions will be freed when the UIO device file is + closed. When no processes are holding the device file open, the address + returned to userspace is DMA_ERROR_CODE. + /para +/sect1 + /chapter chapter id=userspace_driver xreflabel=Writing a driver in user space -- 1.7.5.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/3] uio: uio_pruss: port to AM33xx
On Wed, Sep 26, 2012 at 09:44:29AM -0400, Matt Porter wrote:
Add ifdefery hacks to only use SRAM on Davinci. This
needs to be cleaned up with a sane generic SRAM allocator
(like the DT based driver available that can't be used on
Davinci which is just starting DT conversion) before it
can go upstream.
I agree with that ;-)
Needing lots of ifdefs in *.c files is usually a bad sign...
Thanks anyway for posting, it's good to have things like that in the archives.
And please make driver code and documentation updates separate patches in the
future.
Hans
Adds DT, pinctrl, and runtime PM support for use on
AM33xx.
Signed-off-by: Matt Porter mpor...@ti.com
---
Documentation/devicetree/bindings/uio/pruss.txt| 17 ++
.../devicetree/bindings/uio/uio_pruss.txt | 17 ++
drivers/uio/Kconfig|4 +-
drivers/uio/uio_pruss.c| 63
+++-
4 files changed, 98 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/uio/pruss.txt
create mode 100644 Documentation/devicetree/bindings/uio/uio_pruss.txt
diff --git a/Documentation/devicetree/bindings/uio/pruss.txt
b/Documentation/devicetree/bindings/uio/pruss.txt
new file mode 100644
index 000..2ac45c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/uio/pruss.txt
@@ -0,0 +1,17 @@
+TI PRUSS device
+
+Required properties:
+- compatible :
+ - ti,pruss-v1 for AM18xx/OMAP-L138/DA850
+ - ti,pruss-v2 for AM33xx.
+- ti,pintc-offset : Offset of the PINTC from the PRUSS address base
+- ti,hwmods: Name of the hwmod associated to the PRUSS
+
+Example:
+
+pruss: pruss@4a30 {
+ compatible = ti,pruss-v2;
+ ti,hwmods = pruss;
+ reg = 0x4a30 0x08;
+ ti,pintc-offset = 0x2;
+};
diff --git a/Documentation/devicetree/bindings/uio/uio_pruss.txt
b/Documentation/devicetree/bindings/uio/uio_pruss.txt
new file mode 100644
index 000..2ac45c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/uio/uio_pruss.txt
@@ -0,0 +1,17 @@
+TI PRUSS device
+
+Required properties:
+- compatible :
+ - ti,pruss-v1 for AM18xx/OMAP-L138/DA850
+ - ti,pruss-v2 for AM33xx.
+- ti,pintc-offset : Offset of the PINTC from the PRUSS address base
+- ti,hwmods: Name of the hwmod associated to the PRUSS
+
+Example:
+
+pruss: pruss@4a30 {
+ compatible = ti,pruss-v2;
+ ti,hwmods = pruss;
+ reg = 0x4a30 0x08;
+ ti,pintc-offset = 0x2;
+};
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 6f3ea9b..8da7d9b 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -96,9 +96,9 @@ config UIO_NETX
config UIO_PRUSS
tristate Texas Instruments PRUSS driver
- depends on ARCH_DAVINCI_DA850
+ depends on ARCH_DAVINCI_DA850 || SOC_AM33XX
help
- PRUSS driver for OMAPL138/DA850/AM18XX devices
+ PRUSS driver for OMAPL138/DA850/AM18XX and AM33XX devices
PRUSS driver requires user space components, examples and user space
driver is available from below SVN repo - you may use anonymous login
diff --git a/drivers/uio/uio_pruss.c b/drivers/uio/uio_pruss.c
index 33a7a27..326ce40 100644
--- a/drivers/uio/uio_pruss.c
+++ b/drivers/uio/uio_pruss.c
@@ -25,7 +25,15 @@
#include linux/clk.h
#include linux/dma-mapping.h
#include linux/slab.h
+#include linux/of_address.h
+#include linux/of_device.h
+#include linux/pinctrl/consumer.h
+#include linux/err.h
+#include linux/pm_runtime.h
+
+#ifdef CONFIG_ARCH_DAVINCI_DA850
#include mach/sram.h
+#endif
#define DRV_NAME pruss_uio
#define DRV_VERSION 1.0
@@ -105,8 +113,10 @@ static void pruss_cleanup(struct platform_device *dev,
dma_free_coherent(dev-dev, extram_pool_sz, gdev-ddr_vaddr,
gdev-ddr_paddr);
}
+#ifdef CONFIG_ARCH_DAVINCI_DA850
if (gdev-sram_vaddr)
sram_free(gdev-sram_vaddr, sram_pool_sz);
+#endif
kfree(gdev-info);
clk_put(gdev-pruss_clk);
kfree(gdev);
@@ -117,8 +127,10 @@ static int __devinit pruss_probe(struct platform_device
*dev)
struct uio_info *p;
struct uio_pruss_dev *gdev;
struct resource *regs_prussio;
+ struct resource res;
int ret = -ENODEV, cnt = 0, len;
struct uio_pruss_pdata *pdata = dev-dev.platform_data;
+ struct pinctrl *pinctrl;
gdev = kzalloc(sizeof(struct uio_pruss_dev), GFP_KERNEL);
if (!gdev)
@@ -129,6 +141,7 @@ static int __devinit pruss_probe(struct platform_device
*dev)
kfree(gdev);
return -ENOMEM;
}
+#ifdef CONFIG_ARCH_DAVINCI_DA850
/* Power on PRU in case its not done as part of boot-loader */
gdev-pruss_clk = clk_get(dev-dev, pruss);
if (IS_ERR(gdev-pruss_clk)) {
@@ -140,6 +153,28 @@ static int __devinit pruss_probe(struct platform_device
*dev)
} else {
Re: [RFC PATCH 0/2] Add UIO device supporting dynamic memory allocation
On Wed, Sep 12, 2012 at 02:29:43PM +0900, Damian Hobson-Garcia wrote: > Reposting: I realized that this series should have gone out to a broader list. You forgot GregKH, I added him. > My apologies to those who those who will recieve a duplicate post. > > Hello all, > > I've been using this UIO driver for allocation/deallocation > of memory regions through an IOMMU via the dma-mapping API, but > it seems that it would be more generally useful for userspace drivers > to access CMA memory regions. I don't know if it's useful to try to add > this functionality into the core uio driver or not, so for now I've kept > all dynamic memory handling in the specific device driver. > > The number and size of the dynamically allocatable regions is defined > statically in the device platform data, and the actually memory is > allocated and deallocated when the device is opened/closed. > > Details of the dynamically allocated regions are available from sysfs in > exactly the same was as for static regions. The total number of > dynamic and static regions combined cannot exceed MAX_UIO_MAPS. > > Any comments, especially with regard to exposing the dma-mapping API to > userspace in this way, would be greatly appreciated. > > Damian Hobson-Garcia (2): > Add new uio device for dynamic memory allocation > ARM: shmobile: sh7372: Change VPU UIO to uio_dmem_genirq > > arch/arm/mach-shmobile/setup-sh7372.c | 19 +- > drivers/uio/Kconfig | 16 ++ > drivers/uio/Makefile |1 + > drivers/uio/uio_dmem_genirq.c | 356 > + > include/linux/platform_data/uio_dmem_genirq.h | 26 ++ > 5 files changed, 413 insertions(+), 5 deletions(-) > create mode 100644 drivers/uio/uio_dmem_genirq.c > create mode 100644 include/linux/platform_data/uio_dmem_genirq.h Please add a third patch with a few words for Documentation/DocBook/uio-howto.tmpl since this driver could be useful for other people, too. Otherwise, looks good to me. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] Add UIO device supporting dynamic memory allocation
On Wed, Sep 12, 2012 at 02:29:43PM +0900, Damian Hobson-Garcia wrote: Reposting: I realized that this series should have gone out to a broader list. You forgot GregKH, I added him. My apologies to those who those who will recieve a duplicate post. Hello all, I've been using this UIO driver for allocation/deallocation of memory regions through an IOMMU via the dma-mapping API, but it seems that it would be more generally useful for userspace drivers to access CMA memory regions. I don't know if it's useful to try to add this functionality into the core uio driver or not, so for now I've kept all dynamic memory handling in the specific device driver. The number and size of the dynamically allocatable regions is defined statically in the device platform data, and the actually memory is allocated and deallocated when the device is opened/closed. Details of the dynamically allocated regions are available from sysfs in exactly the same was as for static regions. The total number of dynamic and static regions combined cannot exceed MAX_UIO_MAPS. Any comments, especially with regard to exposing the dma-mapping API to userspace in this way, would be greatly appreciated. Damian Hobson-Garcia (2): Add new uio device for dynamic memory allocation ARM: shmobile: sh7372: Change VPU UIO to uio_dmem_genirq arch/arm/mach-shmobile/setup-sh7372.c | 19 +- drivers/uio/Kconfig | 16 ++ drivers/uio/Makefile |1 + drivers/uio/uio_dmem_genirq.c | 356 + include/linux/platform_data/uio_dmem_genirq.h | 26 ++ 5 files changed, 413 insertions(+), 5 deletions(-) create mode 100644 drivers/uio/uio_dmem_genirq.c create mode 100644 include/linux/platform_data/uio_dmem_genirq.h Please add a third patch with a few words for Documentation/DocBook/uio-howto.tmpl since this driver could be useful for other people, too. Otherwise, looks good to me. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Using uio_pdrv to create an platform device for an FPGA, mmap() fails
On Thu, Aug 30, 2012 at 11:03:04PM +, Worth, Kevin wrote: > > > >> but the code seems like > >> possibly useful sample/example code. > > > >That is another good argument. > > Perhaps this could be genericized to be a generic "Memory Map Userspace > IO Device" that takes a base address and a length in config (since those > are really the only things that are particular to my device/usage). I don't see a point in making it more generic than it already is. Other people's devices generate interrupts or have several mappings. Since these drivers are so small, especially when using uio_pdrv, it doesn't make sense to make it more generic. > Could be enhanced to allow for additional maps, etc. or just serve as a > working example. Docs could then also simply refer to this as an example > of a device that uses the uio_pdrv driver. Just make your driver clean and simple, then refer to it in the docs as one possible example. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Using uio_pdrv to create an platform device for an FPGA, mmap() fails
On Thu, Aug 30, 2012 at 11:03:04PM +, Worth, Kevin wrote: but the code seems like possibly useful sample/example code. That is another good argument. Perhaps this could be genericized to be a generic Memory Map Userspace IO Device that takes a base address and a length in config (since those are really the only things that are particular to my device/usage). I don't see a point in making it more generic than it already is. Other people's devices generate interrupts or have several mappings. Since these drivers are so small, especially when using uio_pdrv, it doesn't make sense to make it more generic. Could be enhanced to allow for additional maps, etc. or just serve as a working example. Docs could then also simply refer to this as an example of a device that uses the uio_pdrv driver. Just make your driver clean and simple, then refer to it in the docs as one possible example. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Using uio_pdrv to create an platform device for an FPGA, mmap() fails
[Added Greg Kroah-Hartman to Cc:]
On Thu, Aug 30, 2012 at 08:10:11PM +, Worth, Kevin wrote:
> >> Thanks for the reply, Hans. Your question about opening /dev/uio0 O_RDWR
> >> prompted me to check out how I was creating /dev/uio0 ... my system
> >> isn't using udev, and I was accidentally creating it with major/minor
> >> number 254/0 instead of the correct 253/0 (found by looking at
> >> /proc/devices). Fixed that and the mmap() call started working.
> >
> >Good.
> >
> >>
> >> Verified that if /dev/uio0 has permissions 0644, root can open it O_RDWR
> >> and mmap PROT_READ | PROT_WRITE using the below code and write to an
> >> address within my memory map. Of course this contradicts the statement
> >> "/dev/uioX is a read-only file" in the UIO howto.
> >
> >You're right. That wants to be fixed...
> >
> >>
> >> Including my updated, tested code for completeness.
> >> Note I also cleaned up the device registration a little by
> >> using a different platform_device_register_ call and removing fields
> >> in the struct uio_info that get filled in by uio_pdrv automatically.
> >
> >If you want to have that included in the mainline, please choose a more
> >descriptive name than "myfpga" and send a proper patch.
>
> I wasn't sure about submitting as a patch since it's for a custom FPGA
> that I don't expect the community will be using,
That doesn't matter. If it helps YOU that the code is maintained in mainline,
post it.
> but the code seems like
> possibly useful sample/example code.
That is another good argument.
> Perhaps patching the HOWTO like
> http://www.kernel.org/doc/htmldocs/uio-howto.html#uio_pci_generic_example
> is the right approach?
Oh, if you could hack up a patch for the documentation, that would be great.
But please make it a second patch, don't mix it with your driver code.
Thanks,
Hans
>
> >
> >Thanks,
> >Hans
> >
> >>
> >> -Kevin
> >>
> >> # lsuio -m -v
> >> uio0: name=uio_myfpga, version=0.1, events=0
> >> map[0]: addr=0xD000, size=262144, mmap test: OK
> >> Device attributes:
> >> uevent=DRIVER=uio_pdrv
> >> modalias=platform:uio_pdrv
> >>
> >> --Kernelspace--
> >> #include
> >> #include
> >> #include
> >>
> >> #define MYFPGA_BASE 0xd000 // 3G
> >> #define MYFPGA_SIZE 0x0004 // 256k
> >>
> >> static struct resource myfpga_resources[] = {
> >> {
> >> .start = MYFPGA_BASE,
> >> .end = MYFPGA_BASE + MYFPGA_SIZE - 1,
> >> .name = "myfpga",
> >> .flags = IORESOURCE_MEM
> >> }
> >> };
> >>
> >> static struct uio_info myfpga_uio_info = {
> >>.name = "uio_myfpga",
> >>.version = "0.1",
> >> };
> >>
> >> static struct platform_device *myfpga_uio_pdev;
> >>
> >> static int __init myfpga_init(void)
> >> {
> >> myfpga_uio_pdev = platform_device_register_resndata (NULL,
> >> "uio_pdrv",
> >> -1,
> >> myfpga_resources,
> >> 1,
> >> _uio_info,
> >> sizeof(struct
> >> uio_info)
> >> );
> >> if (IS_ERR(myfpga_uio_pdev)) {
> >> return PTR_ERR(myfpga_uio_pdev);
> >> }
> >>
> >> return 0;
> >> }
> >>
> >> static void __exit myfpga_exit(void)
> >> {
> >> platform_device_unregister(myfpga_uio_pdev);
> >> }
> >>
> >> module_init(myfpga_init);
> >> module_exit(myfpga_exit);
> >>
> >> --Userspace---
> >> #include
> >> #include
> >> #include
> >>
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >>
> >> #define MYFPGA_BASE 0xd000 // 3G
> >> #define MYFPGA_SIZE 0x0004 // 256k
> >> #define MYFPGA_MAP_NUM 0 // First and only defined map
> >>
> >> #define BIT32(n) (1 << (n))
> >>
> >> /* Use mmap()'ped address "iomem", not physical MYFPGA address */
> >> #define MYFPGA_REG(iomem) (volatile uint32_t*)(iomem + 0x8) // Third
> >> 32-bit reg
> >>
> >> int main (int argc, char *argv[])
> >> {
> >> int fd;
> >> void *iomem;
> >> fd = open("/dev/uio0", O_RDWR|O_SYNC);
> >> if (fd < 0) {
> >> printf("failed to open /dev/uio0, quitting\n");
> >> return -1;
> >> }
> >> /* Note offset has a special meaning with uio devices */
> >> iomem = mmap(NULL, MYFPGA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
> >> MYFPGA_MAP_NUM * getpagesize());
> >> if (iomem == MAP_FAILED) {
> >> printf("mmap failed, quitting\n");
> >> close(fd);
> >> return -2;
> >> }
> >>
> >> /* Set bit 5 of MYFPGA_REG register */
> >> *MYFPGA_REG(iomem) |= BIT32(5);
> >>
> >> munmap(iomem, MYFPGA_SIZE);
> >>
Re: Using uio_pdrv to create an platform device for an FPGA, mmap() fails
On Thu, Aug 30, 2012 at 06:36:53PM +, Worth, Kevin wrote:
> Thanks for the reply, Hans. Your question about opening /dev/uio0 O_RDWR
> prompted me to check out how I was creating /dev/uio0 ... my system
> isn't using udev, and I was accidentally creating it with major/minor
> number 254/0 instead of the correct 253/0 (found by looking at
> /proc/devices). Fixed that and the mmap() call started working.
Good.
>
> Verified that if /dev/uio0 has permissions 0644, root can open it O_RDWR
> and mmap PROT_READ | PROT_WRITE using the below code and write to an
> address within my memory map. Of course this contradicts the statement
> "/dev/uioX is a read-only file" in the UIO howto.
You're right. That wants to be fixed...
>
> Including my updated, tested code for completeness.
> Note I also cleaned up the device registration a little by
> using a different platform_device_register_ call and removing fields
> in the struct uio_info that get filled in by uio_pdrv automatically.
If you want to have that included in the mainline, please choose a more
descriptive name than "myfpga" and send a proper patch.
Thanks,
Hans
>
> -Kevin
>
> # lsuio -m -v
> uio0: name=uio_myfpga, version=0.1, events=0
> map[0]: addr=0xD000, size=262144, mmap test: OK
> Device attributes:
> uevent=DRIVER=uio_pdrv
> modalias=platform:uio_pdrv
>
> --Kernelspace--
> #include
> #include
> #include
>
> #define MYFPGA_BASE 0xd000 // 3G
> #define MYFPGA_SIZE 0x0004 // 256k
>
> static struct resource myfpga_resources[] = {
> {
> .start = MYFPGA_BASE,
> .end = MYFPGA_BASE + MYFPGA_SIZE - 1,
> .name = "myfpga",
> .flags = IORESOURCE_MEM
> }
> };
>
> static struct uio_info myfpga_uio_info = {
>.name = "uio_myfpga",
>.version = "0.1",
> };
>
> static struct platform_device *myfpga_uio_pdev;
>
> static int __init myfpga_init(void)
> {
> myfpga_uio_pdev = platform_device_register_resndata (NULL,
> "uio_pdrv",
> -1,
> myfpga_resources,
> 1,
> _uio_info,
> sizeof(struct
> uio_info)
> );
> if (IS_ERR(myfpga_uio_pdev)) {
> return PTR_ERR(myfpga_uio_pdev);
> }
>
> return 0;
> }
>
> static void __exit myfpga_exit(void)
> {
> platform_device_unregister(myfpga_uio_pdev);
> }
>
> module_init(myfpga_init);
> module_exit(myfpga_exit);
>
> --Userspace---
> #include
> #include
> #include
>
> #include
> #include
> #include
> #include
> #include
> #include
> #include
>
> #define MYFPGA_BASE 0xd000 // 3G
> #define MYFPGA_SIZE 0x0004 // 256k
> #define MYFPGA_MAP_NUM 0 // First and only defined map
>
> #define BIT32(n) (1 << (n))
>
> /* Use mmap()'ped address "iomem", not physical MYFPGA address */
> #define MYFPGA_REG(iomem) (volatile uint32_t*)(iomem + 0x8) // Third 32-bit
> reg
>
> int main (int argc, char *argv[])
> {
> int fd;
> void *iomem;
> fd = open("/dev/uio0", O_RDWR|O_SYNC);
> if (fd < 0) {
> printf("failed to open /dev/uio0, quitting\n");
> return -1;
> }
> /* Note offset has a special meaning with uio devices */
> iomem = mmap(NULL, MYFPGA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
> MYFPGA_MAP_NUM * getpagesize());
> if (iomem == MAP_FAILED) {
> printf("mmap failed, quitting\n");
> close(fd);
> return -2;
> }
>
> /* Set bit 5 of MYFPGA_REG register */
> *MYFPGA_REG(iomem) |= BIT32(5);
>
> munmap(iomem, MYFPGA_SIZE);
> close(fd);
> return 0;
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Using uio_pdrv to create an platform device for an FPGA, mmap() fails
On Thu, Aug 30, 2012 at 06:36:53PM +, Worth, Kevin wrote:
Thanks for the reply, Hans. Your question about opening /dev/uio0 O_RDWR
prompted me to check out how I was creating /dev/uio0 ... my system
isn't using udev, and I was accidentally creating it with major/minor
number 254/0 instead of the correct 253/0 (found by looking at
/proc/devices). Fixed that and the mmap() call started working.
Good.
Verified that if /dev/uio0 has permissions 0644, root can open it O_RDWR
and mmap PROT_READ | PROT_WRITE using the below code and write to an
address within my memory map. Of course this contradicts the statement
/dev/uioX is a read-only file in the UIO howto.
You're right. That wants to be fixed...
Including my updated, tested code for completeness.
Note I also cleaned up the device registration a little by
using a different platform_device_register_ call and removing fields
in the struct uio_info that get filled in by uio_pdrv automatically.
If you want to have that included in the mainline, please choose a more
descriptive name than myfpga and send a proper patch.
Thanks,
Hans
-Kevin
# lsuio -m -v
uio0: name=uio_myfpga, version=0.1, events=0
map[0]: addr=0xD000, size=262144, mmap test: OK
Device attributes:
uevent=DRIVER=uio_pdrv
modalias=platform:uio_pdrv
--Kernelspace--
#include linux/platform_device.h
#include linux/uio_driver.h
#include linux/module.h
#define MYFPGA_BASE 0xd000 // 3G
#define MYFPGA_SIZE 0x0004 // 256k
static struct resource myfpga_resources[] = {
{
.start = MYFPGA_BASE,
.end = MYFPGA_BASE + MYFPGA_SIZE - 1,
.name = myfpga,
.flags = IORESOURCE_MEM
}
};
static struct uio_info myfpga_uio_info = {
.name = uio_myfpga,
.version = 0.1,
};
static struct platform_device *myfpga_uio_pdev;
static int __init myfpga_init(void)
{
myfpga_uio_pdev = platform_device_register_resndata (NULL,
uio_pdrv,
-1,
myfpga_resources,
1,
myfpga_uio_info,
sizeof(struct
uio_info)
);
if (IS_ERR(myfpga_uio_pdev)) {
return PTR_ERR(myfpga_uio_pdev);
}
return 0;
}
static void __exit myfpga_exit(void)
{
platform_device_unregister(myfpga_uio_pdev);
}
module_init(myfpga_init);
module_exit(myfpga_exit);
--Userspace---
#include sys/types.h
#include sys/mman.h
#include sys/stat.h
#include dirent.h
#include string.h
#include stdlib.h
#include stdio.h
#include fcntl.h
#include unistd.h
#include stdint.h
#define MYFPGA_BASE 0xd000 // 3G
#define MYFPGA_SIZE 0x0004 // 256k
#define MYFPGA_MAP_NUM 0 // First and only defined map
#define BIT32(n) (1 (n))
/* Use mmap()'ped address iomem, not physical MYFPGA address */
#define MYFPGA_REG(iomem) (volatile uint32_t*)(iomem + 0x8) // Third 32-bit
reg
int main (int argc, char *argv[])
{
int fd;
void *iomem;
fd = open(/dev/uio0, O_RDWR|O_SYNC);
if (fd 0) {
printf(failed to open /dev/uio0, quitting\n);
return -1;
}
/* Note offset has a special meaning with uio devices */
iomem = mmap(NULL, MYFPGA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
MYFPGA_MAP_NUM * getpagesize());
if (iomem == MAP_FAILED) {
printf(mmap failed, quitting\n);
close(fd);
return -2;
}
/* Set bit 5 of MYFPGA_REG register */
*MYFPGA_REG(iomem) |= BIT32(5);
munmap(iomem, MYFPGA_SIZE);
close(fd);
return 0;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Using uio_pdrv to create an platform device for an FPGA, mmap() fails
[Added Greg Kroah-Hartman to Cc:]
On Thu, Aug 30, 2012 at 08:10:11PM +, Worth, Kevin wrote:
Thanks for the reply, Hans. Your question about opening /dev/uio0 O_RDWR
prompted me to check out how I was creating /dev/uio0 ... my system
isn't using udev, and I was accidentally creating it with major/minor
number 254/0 instead of the correct 253/0 (found by looking at
/proc/devices). Fixed that and the mmap() call started working.
Good.
Verified that if /dev/uio0 has permissions 0644, root can open it O_RDWR
and mmap PROT_READ | PROT_WRITE using the below code and write to an
address within my memory map. Of course this contradicts the statement
/dev/uioX is a read-only file in the UIO howto.
You're right. That wants to be fixed...
Including my updated, tested code for completeness.
Note I also cleaned up the device registration a little by
using a different platform_device_register_ call and removing fields
in the struct uio_info that get filled in by uio_pdrv automatically.
If you want to have that included in the mainline, please choose a more
descriptive name than myfpga and send a proper patch.
I wasn't sure about submitting as a patch since it's for a custom FPGA
that I don't expect the community will be using,
That doesn't matter. If it helps YOU that the code is maintained in mainline,
post it.
but the code seems like
possibly useful sample/example code.
That is another good argument.
Perhaps patching the HOWTO like
http://www.kernel.org/doc/htmldocs/uio-howto.html#uio_pci_generic_example
is the right approach?
Oh, if you could hack up a patch for the documentation, that would be great.
But please make it a second patch, don't mix it with your driver code.
Thanks,
Hans
Thanks,
Hans
-Kevin
# lsuio -m -v
uio0: name=uio_myfpga, version=0.1, events=0
map[0]: addr=0xD000, size=262144, mmap test: OK
Device attributes:
uevent=DRIVER=uio_pdrv
modalias=platform:uio_pdrv
--Kernelspace--
#include linux/platform_device.h
#include linux/uio_driver.h
#include linux/module.h
#define MYFPGA_BASE 0xd000 // 3G
#define MYFPGA_SIZE 0x0004 // 256k
static struct resource myfpga_resources[] = {
{
.start = MYFPGA_BASE,
.end = MYFPGA_BASE + MYFPGA_SIZE - 1,
.name = myfpga,
.flags = IORESOURCE_MEM
}
};
static struct uio_info myfpga_uio_info = {
.name = uio_myfpga,
.version = 0.1,
};
static struct platform_device *myfpga_uio_pdev;
static int __init myfpga_init(void)
{
myfpga_uio_pdev = platform_device_register_resndata (NULL,
uio_pdrv,
-1,
myfpga_resources,
1,
myfpga_uio_info,
sizeof(struct
uio_info)
);
if (IS_ERR(myfpga_uio_pdev)) {
return PTR_ERR(myfpga_uio_pdev);
}
return 0;
}
static void __exit myfpga_exit(void)
{
platform_device_unregister(myfpga_uio_pdev);
}
module_init(myfpga_init);
module_exit(myfpga_exit);
--Userspace---
#include sys/types.h
#include sys/mman.h
#include sys/stat.h
#include dirent.h
#include string.h
#include stdlib.h
#include stdio.h
#include fcntl.h
#include unistd.h
#include stdint.h
#define MYFPGA_BASE 0xd000 // 3G
#define MYFPGA_SIZE 0x0004 // 256k
#define MYFPGA_MAP_NUM 0 // First and only defined map
#define BIT32(n) (1 (n))
/* Use mmap()'ped address iomem, not physical MYFPGA address */
#define MYFPGA_REG(iomem) (volatile uint32_t*)(iomem + 0x8) // Third
32-bit reg
int main (int argc, char *argv[])
{
int fd;
void *iomem;
fd = open(/dev/uio0, O_RDWR|O_SYNC);
if (fd 0) {
printf(failed to open /dev/uio0, quitting\n);
return -1;
}
/* Note offset has a special meaning with uio devices */
iomem = mmap(NULL, MYFPGA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
MYFPGA_MAP_NUM * getpagesize());
if (iomem == MAP_FAILED) {
printf(mmap failed, quitting\n);
close(fd);
return -2;
}
/* Set bit 5 of MYFPGA_REG register */
*MYFPGA_REG(iomem) |= BIT32(5);
munmap(iomem, MYFPGA_SIZE);
close(fd);
return 0;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
Re: Using uio_pdrv to create an platform device for an FPGA, mmap() fails
[Added driver author to Cc:]
On Wed, Aug 29, 2012 at 11:19:00PM +, Worth, Kevin wrote:
> I have below what appears to be a mostly-functional device using the UIO
> Platform Driver. The /sys entries I'd expect appear, /proc/iomem contains "
> d000-dfff : myfpga", and lsuio sees the properties that I've set.
> However an mmap() from userspace (either my test program below or lsuio)
> fails. So close! (at least it would seem)
>
> Finding a good example for this did not come easily (which is why I'm hitting
> LKML, since I know it'll get a lot of eyeballs and be archived). Maybe there
> was something obvious I missed, but the only "official" documentation I could
> locate was http://www.kernel.org/doc/htmldocs/uio-howto.html#using_uio_pdrv ,
> and most everything else was snippets from presentations, papers, and forums
> that lacked completeness.
>
> Thanks for the help any might be able to offer.
> Please CC me on replies as I'm not ready to drink from the fire hose that is
> an LKML subscription.
>
> # lsuio -v -m
> uio0: name=uio_myfpga, version=0.1, events=0
> map[0]: addr=0xD000, size=4096, mmap test: FAILED
> Device attributes:
> uevent=DRIVER=uio_pdrv
> modalias=platform:uio_pdrv
>
> --Kernelspace portion---
>
> #include
> #include
> #include
>
> #define MYFPGA_BASE 0xd000 // 3G
> #define MYFPGA_SIZE 0x0004 // 256k
>
> static struct resource myfpga_resources[] = {
> {
> .start = MYFPGA_BASE,
> .end= MYFPGA_BASE + MYFPGA_SIZE - 1,
> .name = "myfpga",
> .flags = IORESOURCE_MEM
> }
> };
>
> static struct uio_info myfpga_uio_info = {
>.name = "uio_myfpga",
>.version = "0.1",
>.irq = UIO_IRQ_CUSTOM,
>.mem = {
> {
> .name = "myfpga",
> .memtype = UIO_MEM_PHYS,
> .addr = MYFPGA_BASE,
> .size = MYFPGA_SIZE
> }
> }
> };
>
> static struct platform_device_info myfpga_uio_pdevinfo = {
> .name = "uio_pdrv",
> .id = -1,
> .res = myfpga_resources,
> .num_res = 1,
> .data = _uio_info,
> .size_data = sizeof(struct uio_info)
> };
>
> static struct platform_device *myfpga_uio_pdev;
>
> static int __init myfpga_init(void)
> {
> myfpga_uio_pdev = platform_device_register_full(_uio_pdevinfo);
> if (IS_ERR(myfpga_uio_pdev)) {
> return PTR_ERR(myfpga_uio_pdev);
> }
>
> return 0;
> }
>
> static void __exit myfpga_exit(void)
> {
> platform_device_unregister(myfpga_uio_pdev);
> }
>
> module_init(myfpga_init);
> module_exit(myfpga_exit);
>
> --Userspace portion---
>
> #include
> #include
> #include
>
> #include
> #include
> #include
> #include
> #include
> #include
>
> #define MYFPGA_BASE 0xd000 // 3G
> #define MYFPGA_SIZE 0x0004 // 256k
> #define MYFPGA_UIO_NUM 0 // uio0
That's misleading regarding its use below. The factor you need for mmap
is the number of the mapping, not uio0, uio1...
>
> int main (int argc, char *argv[])
> {
> int fd;
> void *iomem;
> fd = open("/dev/uio0", O_RDWR|O_SYNC);
Does it work with O_RDWR ?
Thanks,
Hans
> if (fd < 0) {
> printf("failed to open /dev/uio0, quitting\n");
> return -1;
> }
> /* Note offset has a special meaning with uio devices */
> iomem = mmap(NULL, MYFPGA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
> MYFPGA_UIO_NUM * getpagesize());
> if (iomem == MAP_FAILED) {
> printf("mmap failed, quitting\n");
> close(fd);
> return -2;
> }
> printf("mmap successful!\n");
> munmap(iomem, MYFPGA_SIZE);
> close(fd);
> return 0;
> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Using uio_pdrv to create an platform device for an FPGA, mmap() fails
[Added driver author to Cc:]
On Wed, Aug 29, 2012 at 11:19:00PM +, Worth, Kevin wrote:
I have below what appears to be a mostly-functional device using the UIO
Platform Driver. The /sys entries I'd expect appear, /proc/iomem contains
d000-dfff : myfpga, and lsuio sees the properties that I've set.
However an mmap() from userspace (either my test program below or lsuio)
fails. So close! (at least it would seem)
Finding a good example for this did not come easily (which is why I'm hitting
LKML, since I know it'll get a lot of eyeballs and be archived). Maybe there
was something obvious I missed, but the only official documentation I could
locate was http://www.kernel.org/doc/htmldocs/uio-howto.html#using_uio_pdrv ,
and most everything else was snippets from presentations, papers, and forums
that lacked completeness.
Thanks for the help any might be able to offer.
Please CC me on replies as I'm not ready to drink from the fire hose that is
an LKML subscription.
# lsuio -v -m
uio0: name=uio_myfpga, version=0.1, events=0
map[0]: addr=0xD000, size=4096, mmap test: FAILED
Device attributes:
uevent=DRIVER=uio_pdrv
modalias=platform:uio_pdrv
--Kernelspace portion---
#include linux/platform_device.h
#include linux/uio_driver.h
#include linux/module.h
#define MYFPGA_BASE 0xd000 // 3G
#define MYFPGA_SIZE 0x0004 // 256k
static struct resource myfpga_resources[] = {
{
.start = MYFPGA_BASE,
.end= MYFPGA_BASE + MYFPGA_SIZE - 1,
.name = myfpga,
.flags = IORESOURCE_MEM
}
};
static struct uio_info myfpga_uio_info = {
.name = uio_myfpga,
.version = 0.1,
.irq = UIO_IRQ_CUSTOM,
.mem = {
{
.name = myfpga,
.memtype = UIO_MEM_PHYS,
.addr = MYFPGA_BASE,
.size = MYFPGA_SIZE
}
}
};
static struct platform_device_info myfpga_uio_pdevinfo = {
.name = uio_pdrv,
.id = -1,
.res = myfpga_resources,
.num_res = 1,
.data = myfpga_uio_info,
.size_data = sizeof(struct uio_info)
};
static struct platform_device *myfpga_uio_pdev;
static int __init myfpga_init(void)
{
myfpga_uio_pdev = platform_device_register_full(myfpga_uio_pdevinfo);
if (IS_ERR(myfpga_uio_pdev)) {
return PTR_ERR(myfpga_uio_pdev);
}
return 0;
}
static void __exit myfpga_exit(void)
{
platform_device_unregister(myfpga_uio_pdev);
}
module_init(myfpga_init);
module_exit(myfpga_exit);
--Userspace portion---
#include sys/types.h
#include sys/mman.h
#include sys/stat.h
#include dirent.h
#include string.h
#include stdlib.h
#include stdio.h
#include fcntl.h
#include unistd.h
#define MYFPGA_BASE 0xd000 // 3G
#define MYFPGA_SIZE 0x0004 // 256k
#define MYFPGA_UIO_NUM 0 // uio0
That's misleading regarding its use below. The factor you need for mmap
is the number of the mapping, not uio0, uio1...
int main (int argc, char *argv[])
{
int fd;
void *iomem;
fd = open(/dev/uio0, O_RDWR|O_SYNC);
Does it work with O_RDWR ?
Thanks,
Hans
if (fd 0) {
printf(failed to open /dev/uio0, quitting\n);
return -1;
}
/* Note offset has a special meaning with uio devices */
iomem = mmap(NULL, MYFPGA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
MYFPGA_UIO_NUM * getpagesize());
if (iomem == MAP_FAILED) {
printf(mmap failed, quitting\n);
close(fd);
return -2;
}
printf(mmap successful!\n);
munmap(iomem, MYFPGA_SIZE);
close(fd);
return 0;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: UIO: missing resource mapping
On Wed, Jul 18, 2012 at 12:40:47PM +0200, Dominic Eschweiler wrote:
> Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
> > Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
> > and post it for review.
> >
>
> Here we go ...
Thank you very much for your work. I'm really sorry for the long delay,
but I was busy finishing a project because I go to vacation tomorrow.
Sorry, that might cause further delay since I don't know yet how often
I can read my mail...
Greg, can you review the next one?
Here's a first review.
Thanks,
Hans
> >
> Signed-off-by: Dominic Eschweiler
> diff --git a/drivers/uio/uio_pci_generic.c
> b/drivers/uio/uio_pci_generic.c
> index 0bd08ef..e25991e 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -25,10 +25,12 @@
> #include
> #include
>
> -#define DRIVER_VERSION "0.01.0"
> +#define DRIVER_VERSION "0.02.0"
> #define DRIVER_AUTHOR"Michael S. Tsirkin "
> #define DRIVER_DESC "Generic UIO driver for PCI 2.3 devices"
>
> +#define DRV_NAME "uio_pci_generic"
> +
> struct uio_pci_generic_dev {
> struct uio_info info;
> struct pci_dev *pdev;
> @@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
> {
> struct uio_pci_generic_dev *gdev;
> int err;
> + int i;
>
> err = pci_enable_device(pdev);
> if (err) {
> @@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
> }
>
> if (!pdev->irq) {
> - dev_warn(>dev, "No IRQ assigned to device: "
> - "no support for interrupts?\n");
> + dev_warn(>dev, "No IRQ assigned to device: no support for
> interrupts?\n");
Please configure your mail client not to break lines when sending a patch.
It can't be applied like this.
Why did you make that change anyway? If it's just coding style, please send
another patch, don't mix functional changes with coding style fixes.
> pci_disable_device(pdev);
> return -ENODEV;
> }
> @@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
> gdev->info.handler = irqhandler;
> gdev->pdev = pdev;
>
> + /* request regions */
> + err = pci_request_regions(pdev, DRV_NAME);
> + if (err) {
> + dev_err(>dev, "Couldn't get PCI resources, aborting\n");
> + return err;
> + }
> +
> + /* create attributes for BAR mappings */
> + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> + if (pdev->resource[i].flags &&
> + (pdev->resource[i].flags & IORESOURCE_MEM)) {
> + gdev->info.mem[i].addr = pci_resource_start(pdev, i);
> + gdev->info.mem[i].size = pci_resource_len(pdev, i);
> + gdev->info.mem[i].internal_addr = NULL;
> + gdev->info.mem[i].memtype = UIO_MEM_PHYS;
> + }
> + }
> +
> if (uio_register_device(>dev, >info))
> goto err_register;
> pci_set_drvdata(pdev, gdev);
>
> + pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",
Please use dev_info()
> + pdev->vendor, pdev->device);
> +
> return 0;
> err_register:
> kfree(gdev);
> @@ -107,17 +130,21 @@ err_verify:
> static void remove(struct pci_dev *pdev)
> {
> struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
> -
> uio_unregister_device(>info);
> +
> + pci_release_regions(pdev);
> pci_disable_device(pdev);
> kfree(gdev);
> +
> + pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",
ditto
> + pdev->vendor, pdev->device);
> }
>
> static struct pci_driver driver = {
> - .name = "uio_pci_generic",
> + .name = DRV_NAME,
> .id_table = NULL, /* only dynamic id's */
> - .probe = probe,
> - .remove = remove,
> + .probe= probe,
> + .remove = remove,
As above: Please put coding style fixes in an extra patch (if you really
insist on tabs instead of spaces...)
> };
>
> static int __init init(void)
>
> --
> Gruß
> Dominic
>
> Frankfurt Institute for Advanced Studies (FIAS)
> Ruth-Moufang-Straße 1
> D-60438 Frankfurt am Main
> Germany
>
> Phone: +49 69 79844114
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: UIO: missing resource mapping
On Wed, Jul 18, 2012 at 12:40:47PM +0200, Dominic Eschweiler wrote:
Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
and post it for review.
Here we go ...
Thank you very much for your work. I'm really sorry for the long delay,
but I was busy finishing a project because I go to vacation tomorrow.
Sorry, that might cause further delay since I don't know yet how often
I can read my mail...
Greg, can you review the next one?
Here's a first review.
Thanks,
Hans
Signed-off-by: Dominic Eschweiler eschwei...@fias.uni-frankfurt.de
diff --git a/drivers/uio/uio_pci_generic.c
b/drivers/uio/uio_pci_generic.c
index 0bd08ef..e25991e 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -25,10 +25,12 @@
#include linux/slab.h
#include linux/uio_driver.h
-#define DRIVER_VERSION 0.01.0
+#define DRIVER_VERSION 0.02.0
#define DRIVER_AUTHORMichael S. Tsirkin m...@redhat.com
#define DRIVER_DESC Generic UIO driver for PCI 2.3 devices
+#define DRV_NAME uio_pci_generic
+
struct uio_pci_generic_dev {
struct uio_info info;
struct pci_dev *pdev;
@@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
{
struct uio_pci_generic_dev *gdev;
int err;
+ int i;
err = pci_enable_device(pdev);
if (err) {
@@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
}
if (!pdev-irq) {
- dev_warn(pdev-dev, No IRQ assigned to device:
- no support for interrupts?\n);
+ dev_warn(pdev-dev, No IRQ assigned to device: no support for
interrupts?\n);
Please configure your mail client not to break lines when sending a patch.
It can't be applied like this.
Why did you make that change anyway? If it's just coding style, please send
another patch, don't mix functional changes with coding style fixes.
pci_disable_device(pdev);
return -ENODEV;
}
@@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
gdev-info.handler = irqhandler;
gdev-pdev = pdev;
+ /* request regions */
+ err = pci_request_regions(pdev, DRV_NAME);
+ if (err) {
+ dev_err(pdev-dev, Couldn't get PCI resources, aborting\n);
+ return err;
+ }
+
+ /* create attributes for BAR mappings */
+ for (i = 0; i PCI_NUM_RESOURCES; i++) {
+ if (pdev-resource[i].flags
+ (pdev-resource[i].flags IORESOURCE_MEM)) {
+ gdev-info.mem[i].addr = pci_resource_start(pdev, i);
+ gdev-info.mem[i].size = pci_resource_len(pdev, i);
+ gdev-info.mem[i].internal_addr = NULL;
+ gdev-info.mem[i].memtype = UIO_MEM_PHYS;
+ }
+ }
+
if (uio_register_device(pdev-dev, gdev-info))
goto err_register;
pci_set_drvdata(pdev, gdev);
+ pr_info(UIO_PCI_GENERIC : initialized new device (%x %x)\n,
Please use dev_info()
+ pdev-vendor, pdev-device);
+
return 0;
err_register:
kfree(gdev);
@@ -107,17 +130,21 @@ err_verify:
static void remove(struct pci_dev *pdev)
{
struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
-
uio_unregister_device(gdev-info);
+
+ pci_release_regions(pdev);
pci_disable_device(pdev);
kfree(gdev);
+
+ pr_info(UIO_PCI_GENERIC : removed device (%x %x)\n,
ditto
+ pdev-vendor, pdev-device);
}
static struct pci_driver driver = {
- .name = uio_pci_generic,
+ .name = DRV_NAME,
.id_table = NULL, /* only dynamic id's */
- .probe = probe,
- .remove = remove,
+ .probe= probe,
+ .remove = remove,
As above: Please put coding style fixes in an extra patch (if you really
insist on tabs instead of spaces...)
};
static int __init init(void)
--
Gruß
Dominic
Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany
Phone: +49 69 79844114
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: UIO: missing resource mapping
On Wed, Jul 18, 2012 at 12:40:47PM +0200, Dominic Eschweiler wrote:
> Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
> > Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
> > and post it for review.
> >
>
> Here we go ...
Great! I'm a bit under time pressure with my current work ATM. A call to
open("/dev/hansjkoch", O_NONBLOCK) only returns -EBUSY. So it'll take me one or
two days to review it thoroughly. But at first sight, it is what I had in mind.
You'll hear from me soon, thanks for your work! Comments and reviews from
others are welcome...
Hans
> >
> Signed-off-by: Dominic Eschweiler
> diff --git a/drivers/uio/uio_pci_generic.c
> b/drivers/uio/uio_pci_generic.c
> index 0bd08ef..e25991e 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -25,10 +25,12 @@
> #include
> #include
>
> -#define DRIVER_VERSION "0.01.0"
> +#define DRIVER_VERSION "0.02.0"
> #define DRIVER_AUTHOR"Michael S. Tsirkin "
> #define DRIVER_DESC "Generic UIO driver for PCI 2.3 devices"
>
> +#define DRV_NAME "uio_pci_generic"
> +
> struct uio_pci_generic_dev {
> struct uio_info info;
> struct pci_dev *pdev;
> @@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
> {
> struct uio_pci_generic_dev *gdev;
> int err;
> + int i;
>
> err = pci_enable_device(pdev);
> if (err) {
> @@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
> }
>
> if (!pdev->irq) {
> - dev_warn(>dev, "No IRQ assigned to device: "
> - "no support for interrupts?\n");
> + dev_warn(>dev, "No IRQ assigned to device: no support for
> interrupts?\n");
> pci_disable_device(pdev);
> return -ENODEV;
> }
> @@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
> gdev->info.handler = irqhandler;
> gdev->pdev = pdev;
>
> + /* request regions */
> + err = pci_request_regions(pdev, DRV_NAME);
> + if (err) {
> + dev_err(>dev, "Couldn't get PCI resources, aborting\n");
> + return err;
> + }
> +
> + /* create attributes for BAR mappings */
> + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> + if (pdev->resource[i].flags &&
> + (pdev->resource[i].flags & IORESOURCE_MEM)) {
> + gdev->info.mem[i].addr = pci_resource_start(pdev, i);
> + gdev->info.mem[i].size = pci_resource_len(pdev, i);
> + gdev->info.mem[i].internal_addr = NULL;
> + gdev->info.mem[i].memtype = UIO_MEM_PHYS;
> + }
> + }
> +
> if (uio_register_device(>dev, >info))
> goto err_register;
> pci_set_drvdata(pdev, gdev);
>
> + pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",
> + pdev->vendor, pdev->device);
> +
> return 0;
> err_register:
> kfree(gdev);
> @@ -107,17 +130,21 @@ err_verify:
> static void remove(struct pci_dev *pdev)
> {
> struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
> -
> uio_unregister_device(>info);
> +
> + pci_release_regions(pdev);
> pci_disable_device(pdev);
> kfree(gdev);
> +
> + pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",
> + pdev->vendor, pdev->device);
> }
>
> static struct pci_driver driver = {
> - .name = "uio_pci_generic",
> + .name = DRV_NAME,
> .id_table = NULL, /* only dynamic id's */
> - .probe = probe,
> - .remove = remove,
> + .probe= probe,
> + .remove = remove,
> };
>
> static int __init init(void)
>
> --
> Gruß
> Dominic
>
> Frankfurt Institute for Advanced Studies (FIAS)
> Ruth-Moufang-Straße 1
> D-60438 Frankfurt am Main
> Germany
>
> Phone: +49 69 79844114
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: UIO: missing resource mapping
On Wed, Jul 18, 2012 at 12:40:47PM +0200, Dominic Eschweiler wrote:
Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
and post it for review.
Here we go ...
Great! I'm a bit under time pressure with my current work ATM. A call to
open(/dev/hansjkoch, O_NONBLOCK) only returns -EBUSY. So it'll take me one or
two days to review it thoroughly. But at first sight, it is what I had in mind.
You'll hear from me soon, thanks for your work! Comments and reviews from
others are welcome...
Hans
Signed-off-by: Dominic Eschweiler eschwei...@fias.uni-frankfurt.de
diff --git a/drivers/uio/uio_pci_generic.c
b/drivers/uio/uio_pci_generic.c
index 0bd08ef..e25991e 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -25,10 +25,12 @@
#include linux/slab.h
#include linux/uio_driver.h
-#define DRIVER_VERSION 0.01.0
+#define DRIVER_VERSION 0.02.0
#define DRIVER_AUTHORMichael S. Tsirkin m...@redhat.com
#define DRIVER_DESC Generic UIO driver for PCI 2.3 devices
+#define DRV_NAME uio_pci_generic
+
struct uio_pci_generic_dev {
struct uio_info info;
struct pci_dev *pdev;
@@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
{
struct uio_pci_generic_dev *gdev;
int err;
+ int i;
err = pci_enable_device(pdev);
if (err) {
@@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
}
if (!pdev-irq) {
- dev_warn(pdev-dev, No IRQ assigned to device:
- no support for interrupts?\n);
+ dev_warn(pdev-dev, No IRQ assigned to device: no support for
interrupts?\n);
pci_disable_device(pdev);
return -ENODEV;
}
@@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
gdev-info.handler = irqhandler;
gdev-pdev = pdev;
+ /* request regions */
+ err = pci_request_regions(pdev, DRV_NAME);
+ if (err) {
+ dev_err(pdev-dev, Couldn't get PCI resources, aborting\n);
+ return err;
+ }
+
+ /* create attributes for BAR mappings */
+ for (i = 0; i PCI_NUM_RESOURCES; i++) {
+ if (pdev-resource[i].flags
+ (pdev-resource[i].flags IORESOURCE_MEM)) {
+ gdev-info.mem[i].addr = pci_resource_start(pdev, i);
+ gdev-info.mem[i].size = pci_resource_len(pdev, i);
+ gdev-info.mem[i].internal_addr = NULL;
+ gdev-info.mem[i].memtype = UIO_MEM_PHYS;
+ }
+ }
+
if (uio_register_device(pdev-dev, gdev-info))
goto err_register;
pci_set_drvdata(pdev, gdev);
+ pr_info(UIO_PCI_GENERIC : initialized new device (%x %x)\n,
+ pdev-vendor, pdev-device);
+
return 0;
err_register:
kfree(gdev);
@@ -107,17 +130,21 @@ err_verify:
static void remove(struct pci_dev *pdev)
{
struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
-
uio_unregister_device(gdev-info);
+
+ pci_release_regions(pdev);
pci_disable_device(pdev);
kfree(gdev);
+
+ pr_info(UIO_PCI_GENERIC : removed device (%x %x)\n,
+ pdev-vendor, pdev-device);
}
static struct pci_driver driver = {
- .name = uio_pci_generic,
+ .name = DRV_NAME,
.id_table = NULL, /* only dynamic id's */
- .probe = probe,
- .remove = remove,
+ .probe= probe,
+ .remove = remove,
};
static int __init init(void)
--
Gruß
Dominic
Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany
Phone: +49 69 79844114
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: UIO: missing resource mapping
On Mon, Jul 16, 2012 at 08:16:12PM +0200, Dominic Eschweiler wrote: > Am Freitag, den 13.07.2012, 21:19 +0300 schrieb Michael S. Tsirkin: > > > > UIO has the same property, doesn't it? Multiple users can > > access device memory through sysfs. > > > Indeed, that's a similar problem. I haven't tried it (yet), but this > particular problem can maybe circumvented by using mmap with the > MAP_PRIVATE flag. Doing so is the responsibility of the driver > programmer (like Hans already said). Even if that mmap trick does not > work, it is pretty much sure that a BAR is already used by another > program, if a related kernel driver is loaded. In that case the kernel > has a chance to avoid such BAR race conditions by not giving the > possibility to map them to the userspace. Don't make it more complicated than it is. I see no general problem in mapping BARs in uio_pci_generic like in any other UIO PCI driver. > > Nevertheless, I'm pretty sure that the possibility via sysfs to access > BARs, which are already managed by a kernel driver, opens the door for > denial of service attacks. There are also a few other possible attack scenarios, depending on the hardware. That's a general problem of all userspace drivers (e.g. X graphics drivers). You have to make sure access rights are correct. Making a UIO device node available to all normal users is foolishly dangerous. > > On the other hand, I'm quite a newbie on this topic and maybe I don't > see the big picture here. Therefore it is up to you guys to make the > right decision (if needed). Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c and post it for review. Thanks, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: UIO: missing resource mapping
On Mon, Jul 16, 2012 at 08:16:12PM +0200, Dominic Eschweiler wrote: Am Freitag, den 13.07.2012, 21:19 +0300 schrieb Michael S. Tsirkin: UIO has the same property, doesn't it? Multiple users can access device memory through sysfs. Indeed, that's a similar problem. I haven't tried it (yet), but this particular problem can maybe circumvented by using mmap with the MAP_PRIVATE flag. Doing so is the responsibility of the driver programmer (like Hans already said). Even if that mmap trick does not work, it is pretty much sure that a BAR is already used by another program, if a related kernel driver is loaded. In that case the kernel has a chance to avoid such BAR race conditions by not giving the possibility to map them to the userspace. Don't make it more complicated than it is. I see no general problem in mapping BARs in uio_pci_generic like in any other UIO PCI driver. Nevertheless, I'm pretty sure that the possibility via sysfs to access BARs, which are already managed by a kernel driver, opens the door for denial of service attacks. There are also a few other possible attack scenarios, depending on the hardware. That's a general problem of all userspace drivers (e.g. X graphics drivers). You have to make sure access rights are correct. Making a UIO device node available to all normal users is foolishly dangerous. On the other hand, I'm quite a newbie on this topic and maybe I don't see the big picture here. Therefore it is up to you guys to make the right decision (if needed). Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c and post it for review. Thanks, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
