| Message ID | 1438977619-15488-3-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <bigeasy@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 dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <bigeasy@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
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 <bigeasy@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.
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 dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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..
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);
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 <bigeasy@linutronix.de> --- include/linux/dmaengine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)