Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()
On Tue, Aug 11, 2015 at 02:34:29PM +0200, Sebastian Andrzej Siewior wrote: > On 08/11/2015 12:06 PM, Russell King - ARM Linux wrote: > > I think what people need to learn is that an API in the kernel which > > returns an int _can_ fail - it returns an int so it _can_ return an > > error code. If it _can_ return an error code, there _will_ be > > implementations which _do_. > > > > If you don't check the return code, either your code doesn't care whether > > the function was successful or not, or you're playing with fire. This is > > a prime example of playing with fire. > > > > Let's leave the crappy userspace laziness with regard to error checking > > to userspace, and keep it out of the kernel. > > > > Yes, the DMA engine capabilities may not be sufficient to describe every > > detail of DMA engines, but that's absolutely no reason to skimp on error > > checking. Had there been some kind of error checking at the site, this > > problem would have been spotted before the 8250-omap driver was merged. > > Let me disable RX-DMA in 8250-omap code and push that stable. Then we > won't need a special annotation for pause support because it remains > off and is currently about one user. I browsed each driver in > drivers/dma each one which does support pause supports it and all of > them implement it unconditionally (ipu_idmac grabs a mutex first but > this is another story). > Adding error checking to 8250-omap like I have it in #1 and disabling > RX-DMA in case pause fails looks be reasonable since there is nothing > else that can be done I guess. > Once we have the missing piece in omap-dma the RX-DMA can be enabled in > 8250-omap. > Does this sound like a plan we can agree on? Yes sounds good to me.. -- ~Vinod -- 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/3] dma: add __must_check annotation for dmaengine_pause()
On Tue, Aug 11, 2015 at 02:34:29PM +0200, Sebastian Andrzej Siewior wrote: On 08/11/2015 12:06 PM, Russell King - ARM Linux wrote: I think what people need to learn is that an API in the kernel which returns an int _can_ fail - it returns an int so it _can_ return an error code. If it _can_ return an error code, there _will_ be implementations which _do_. If you don't check the return code, either your code doesn't care whether the function was successful or not, or you're playing with fire. This is a prime example of playing with fire. Let's leave the crappy userspace laziness with regard to error checking to userspace, and keep it out of the kernel. Yes, the DMA engine capabilities may not be sufficient to describe every detail of DMA engines, but that's absolutely no reason to skimp on error checking. Had there been some kind of error checking at the site, this problem would have been spotted before the 8250-omap driver was merged. Let me disable RX-DMA in 8250-omap code and push that stable. Then we won't need a special annotation for pause support because it remains off and is currently about one user. I browsed each driver in drivers/dma each one which does support pause supports it and all of them implement it unconditionally (ipu_idmac grabs a mutex first but this is another story). Adding error checking to 8250-omap like I have it in #1 and disabling RX-DMA in case pause fails looks be reasonable since there is nothing else that can be done I guess. Once we have the missing piece in omap-dma the RX-DMA can be enabled in 8250-omap. Does this sound like a plan we can agree on? Yes sounds good to me.. -- ~Vinod -- 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/3] dma: add __must_check annotation for dmaengine_pause()
On 08/11/2015 12:06 PM, Russell King - ARM Linux wrote: > I think what people need to learn is that an API in the kernel which > returns an int _can_ fail - it returns an int so it _can_ return an > error code. If it _can_ return an error code, there _will_ be > implementations which _do_. > > If you don't check the return code, either your code doesn't care whether > the function was successful or not, or you're playing with fire. This is > a prime example of playing with fire. > > Let's leave the crappy userspace laziness with regard to error checking > to userspace, and keep it out of the kernel. > > Yes, the DMA engine capabilities may not be sufficient to describe every > detail of DMA engines, but that's absolutely no reason to skimp on error > checking. Had there been some kind of error checking at the site, this > problem would have been spotted before the 8250-omap driver was merged. Let me disable RX-DMA in 8250-omap code and push that stable. Then we won't need a special annotation for pause support because it remains off and is currently about one user. I browsed each driver in drivers/dma each one which does support pause supports it and all of them implement it unconditionally (ipu_idmac grabs a mutex first but this is another story). Adding error checking to 8250-omap like I have it in #1 and disabling RX-DMA in case pause fails looks be reasonable since there is nothing else that can be done I guess. Once we have the missing piece in omap-dma the RX-DMA can be enabled in 8250-omap. Does this sound like a plan we can agree on? Sebastian -- 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/3] dma: add __must_check annotation for dmaengine_pause()
On Tue, Aug 11, 2015 at 03:28:52PM +0530, Vinod Koul wrote:
> On Fri, Aug 07, 2015 at 10:00:18PM +0200, Sebastian Andrzej Siewior wrote:
> > In 8250-omap I learned it the hard way that ignoring the return code
> > of dmaengine_pause() might be bad because the underlying DMA driver
> > might not support the function at all and so not doing what one is
> > expecting.
> > This patch adds the __must_check annotation as suggested by Russell King.
> >
> > Signed-off-by: Sebastian Andrzej Siewior
> > ---
> > include/linux/dmaengine.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index 8ad9a4e839f6..4eac4716bded 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct
> > dma_chan *chan)
> > return -ENOSYS;
> > }
> >
> > -static inline int dmaengine_pause(struct dma_chan *chan)
> > +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
> > {
> > if (chan->device->device_pause)
> > return chan->device->device_pause(chan);
>
> Give that there are bunch of users of this call which may or maynot be using
> this, I think putting this check is too stringent
I think what people need to learn is that an API in the kernel which
returns an int _can_ fail - it returns an int so it _can_ return an
error code. If it _can_ return an error code, there _will_ be
implementations which _do_.
If you don't check the return code, either your code doesn't care whether
the function was successful or not, or you're playing with fire. This is
a prime example of playing with fire.
Let's leave the crappy userspace laziness with regard to error checking
to userspace, and keep it out of the kernel.
Yes, the DMA engine capabilities may not be sufficient to describe every
detail of DMA engines, but that's absolutely no reason to skimp on error
checking. Had there been some kind of error checking at the site, this
problem would have been spotted before the 8250-omap driver was merged.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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/3] dma: add __must_check annotation for dmaengine_pause()
On Fri, Aug 07, 2015 at 10:00:18PM +0200, Sebastian Andrzej Siewior wrote:
> In 8250-omap I learned it the hard way that ignoring the return code
> of dmaengine_pause() might be bad because the underlying DMA driver
> might not support the function at all and so not doing what one is
> expecting.
> This patch adds the __must_check annotation as suggested by Russell King.
>
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> include/linux/dmaengine.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8ad9a4e839f6..4eac4716bded 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan
> *chan)
> return -ENOSYS;
> }
>
> -static inline int dmaengine_pause(struct dma_chan *chan)
> +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
> {
> if (chan->device->device_pause)
> return chan->device->device_pause(chan);
Give that there are bunch of users of this call which may or maynot be using
this, I think putting this check is too stringent
--
~Vinod
--
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/3] dma: add __must_check annotation for dmaengine_pause()
On Fri, Aug 07, 2015 at 10:00:18PM +0200, Sebastian Andrzej Siewior wrote:
In 8250-omap I learned it the hard way that ignoring the return code
of dmaengine_pause() might be bad because the underlying DMA driver
might not support the function at all and so not doing what one is
expecting.
This patch adds the __must_check annotation as suggested by Russell King.
Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
---
include/linux/dmaengine.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8ad9a4e839f6..4eac4716bded 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan
*chan)
return -ENOSYS;
}
-static inline int dmaengine_pause(struct dma_chan *chan)
+static inline int __must_check dmaengine_pause(struct dma_chan *chan)
{
if (chan-device-device_pause)
return chan-device-device_pause(chan);
Give that there are bunch of users of this call which may or maynot be using
this, I think putting this check is too stringent
--
~Vinod
--
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/3] dma: add __must_check annotation for dmaengine_pause()
On Tue, Aug 11, 2015 at 03:28:52PM +0530, Vinod Koul wrote:
On Fri, Aug 07, 2015 at 10:00:18PM +0200, Sebastian Andrzej Siewior wrote:
In 8250-omap I learned it the hard way that ignoring the return code
of dmaengine_pause() might be bad because the underlying DMA driver
might not support the function at all and so not doing what one is
expecting.
This patch adds the __must_check annotation as suggested by Russell King.
Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
---
include/linux/dmaengine.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8ad9a4e839f6..4eac4716bded 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct
dma_chan *chan)
return -ENOSYS;
}
-static inline int dmaengine_pause(struct dma_chan *chan)
+static inline int __must_check dmaengine_pause(struct dma_chan *chan)
{
if (chan-device-device_pause)
return chan-device-device_pause(chan);
Give that there are bunch of users of this call which may or maynot be using
this, I think putting this check is too stringent
I think what people need to learn is that an API in the kernel which
returns an int _can_ fail - it returns an int so it _can_ return an
error code. If it _can_ return an error code, there _will_ be
implementations which _do_.
If you don't check the return code, either your code doesn't care whether
the function was successful or not, or you're playing with fire. This is
a prime example of playing with fire.
Let's leave the crappy userspace laziness with regard to error checking
to userspace, and keep it out of the kernel.
Yes, the DMA engine capabilities may not be sufficient to describe every
detail of DMA engines, but that's absolutely no reason to skimp on error
checking. Had there been some kind of error checking at the site, this
problem would have been spotted before the 8250-omap driver was merged.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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/3] dma: add __must_check annotation for dmaengine_pause()
On 08/11/2015 12:06 PM, Russell King - ARM Linux wrote: I think what people need to learn is that an API in the kernel which returns an int _can_ fail - it returns an int so it _can_ return an error code. If it _can_ return an error code, there _will_ be implementations which _do_. If you don't check the return code, either your code doesn't care whether the function was successful or not, or you're playing with fire. This is a prime example of playing with fire. Let's leave the crappy userspace laziness with regard to error checking to userspace, and keep it out of the kernel. Yes, the DMA engine capabilities may not be sufficient to describe every detail of DMA engines, but that's absolutely no reason to skimp on error checking. Had there been some kind of error checking at the site, this problem would have been spotted before the 8250-omap driver was merged. Let me disable RX-DMA in 8250-omap code and push that stable. Then we won't need a special annotation for pause support because it remains off and is currently about one user. I browsed each driver in drivers/dma each one which does support pause supports it and all of them implement it unconditionally (ipu_idmac grabs a mutex first but this is another story). Adding error checking to 8250-omap like I have it in #1 and disabling RX-DMA in case pause fails looks be reasonable since there is nothing else that can be done I guess. Once we have the missing piece in omap-dma the RX-DMA can be enabled in 8250-omap. Does this sound like a plan we can agree on? Sebastian -- 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/3] dma: add __must_check annotation for dmaengine_pause()
On 08/07/2015 04:00 PM, Sebastian Andrzej Siewior wrote:
> In 8250-omap I learned it the hard way that ignoring the return code
> of dmaengine_pause() might be bad because the underlying DMA driver
> might not support the function at all and so not doing what one is
> expecting.
> This patch adds the __must_check annotation as suggested by Russell King.
>
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> include/linux/dmaengine.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8ad9a4e839f6..4eac4716bded 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan
> *chan)
> return -ENOSYS;
> }
>
> -static inline int dmaengine_pause(struct dma_chan *chan)
> +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
> {
> if (chan->device->device_pause)
> return chan->device->device_pause(chan);
>
Not that this is your responsibility, Sebastian, but considering there are
fewer than 20 users of dmaengine_pause() in the entire tree, we should add
WARN_ON_ONCE() around those uses with this patch to avoid a bunch needless
one-off "fixes".
Regards,
Peter Hurley
--
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 2/3] dma: add __must_check annotation for dmaengine_pause()
In 8250-omap I learned it the hard way that ignoring the return code
of dmaengine_pause() might be bad because the underlying DMA driver
might not support the function at all and so not doing what one is
expecting.
This patch adds the __must_check annotation as suggested by Russell King.
Signed-off-by: Sebastian Andrzej Siewior
---
include/linux/dmaengine.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8ad9a4e839f6..4eac4716bded 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan
*chan)
return -ENOSYS;
}
-static inline int dmaengine_pause(struct dma_chan *chan)
+static inline int __must_check dmaengine_pause(struct dma_chan *chan)
{
if (chan->device->device_pause)
return chan->device->device_pause(chan);
--
2.5.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: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()
On 08/07/2015 04:00 PM, Sebastian Andrzej Siewior wrote:
In 8250-omap I learned it the hard way that ignoring the return code
of dmaengine_pause() might be bad because the underlying DMA driver
might not support the function at all and so not doing what one is
expecting.
This patch adds the __must_check annotation as suggested by Russell King.
Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
---
include/linux/dmaengine.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8ad9a4e839f6..4eac4716bded 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan
*chan)
return -ENOSYS;
}
-static inline int dmaengine_pause(struct dma_chan *chan)
+static inline int __must_check dmaengine_pause(struct dma_chan *chan)
{
if (chan-device-device_pause)
return chan-device-device_pause(chan);
Not that this is your responsibility, Sebastian, but considering there are
fewer than 20 users of dmaengine_pause() in the entire tree, we should add
WARN_ON_ONCE() around those uses with this patch to avoid a bunch needless
one-off fixes.
Regards,
Peter Hurley
--
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 2/3] dma: add __must_check annotation for dmaengine_pause()
In 8250-omap I learned it the hard way that ignoring the return code
of dmaengine_pause() might be bad because the underlying DMA driver
might not support the function at all and so not doing what one is
expecting.
This patch adds the __must_check annotation as suggested by Russell King.
Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
---
include/linux/dmaengine.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8ad9a4e839f6..4eac4716bded 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan
*chan)
return -ENOSYS;
}
-static inline int dmaengine_pause(struct dma_chan *chan)
+static inline int __must_check dmaengine_pause(struct dma_chan *chan)
{
if (chan-device-device_pause)
return chan-device-device_pause(chan);
--
2.5.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/
