* [PATCH] async-tx: fix buffer sumbission error handling in ipu_idma.c @ 2010-02-10 15:44 Guennadi Liakhovetski 2010-02-10 15:54 ` Dan Williams 0 siblings, 1 reply; 7+ messages in thread From: Guennadi Liakhovetski @ 2010-02-10 15:44 UTC (permalink / raw) To: linux-arm-kernel If submitting new buffer failed, a wrong descriptor gets completed and it doesn't check, if a callback is at all defined, which can lead to an Oops. Fix these bugs and make ipu_update_channel_buffer() void, because it never fails. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- drivers/dma/ipu/ipu_idmac.c | 24 +++++++----------------- 1 files changed, 7 insertions(+), 17 deletions(-) diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c index 9a5bc1a..fbc5d25 100644 --- a/drivers/dma/ipu/ipu_idmac.c +++ b/drivers/dma/ipu/ipu_idmac.c @@ -761,12 +761,10 @@ static void ipu_select_buffer(enum ipu_channel channel, int buffer_n) * @buffer_n: buffer number to update. * 0 or 1 are the only valid values. * @phyaddr: buffer physical address. - * @return: Returns 0 on success or negative error code on failure. This - * function will fail if the buffer is set to ready. */ /* Called under spin_lock(_irqsave)(&ichan->lock) */ -static int ipu_update_channel_buffer(struct idmac_channel *ichan, - int buffer_n, dma_addr_t phyaddr) +static void ipu_update_channel_buffer(struct idmac_channel *ichan, + int buffer_n, dma_addr_t phyaddr) { enum ipu_channel channel = ichan->dma_chan.chan_id; uint32_t reg; @@ -806,8 +804,6 @@ static int ipu_update_channel_buffer(struct idmac_channel *ichan, } spin_unlock_irqrestore(&ipu_data.lock, flags); - - return 0; } /* Called under spin_lock_irqsave(&ichan->lock) */ @@ -827,14 +823,7 @@ static int ipu_submit_buffer(struct idmac_channel *ichan, * could make it conditional on status >= IPU_CHANNEL_ENABLED, but * doing it again shouldn't hurt either. */ - ret = ipu_update_channel_buffer(ichan, buf_idx, - sg_dma_address(sg)); - - if (ret < 0) { - dev_err(dev, "Updating sg %p on channel 0x%x buffer %d failed!\n", - sg, chan_id, buf_idx); - return ret; - } + ipu_update_channel_buffer(ichan, buf_idx, sg_dma_address(sg)); ipu_select_buffer(chan_id, buf_idx); dev_dbg(dev, "Updated sg %p on channel 0x%x buffer %d\n", @@ -1379,10 +1368,11 @@ static irqreturn_t idmac_interrupt(int irq, void *dev_id) if (likely(sgnew) && ipu_submit_buffer(ichan, descnew, sgnew, ichan->active_buffer) < 0) { - callback = desc->txd.callback; - callback_param = desc->txd.callback_param; + callback = descnew->txd.callback; + callback_param = descnew->txd.callback_param; spin_unlock(&ichan->lock); - callback(callback_param); + if (callback) + callback(callback_param); spin_lock(&ichan->lock); } -- 1.6.2.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] async-tx: fix buffer sumbission error handling in ipu_idma.c 2010-02-10 15:44 [PATCH] async-tx: fix buffer sumbission error handling in ipu_idma.c Guennadi Liakhovetski @ 2010-02-10 15:54 ` Dan Williams 2010-02-10 16:32 ` [PATCH v2] " Guennadi Liakhovetski 0 siblings, 1 reply; 7+ messages in thread From: Dan Williams @ 2010-02-10 15:54 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 10, 2010 at 8:44 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > If submitting new buffer failed, a wrong descriptor gets completed and it > doesn't check, if a callback is at all defined, which can lead to an Oops. Fix > these bugs and make ipu_update_channel_buffer() void, because it never fails. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Looks like 2.6.33 material. Shall I tag it for -stable as well? -- Dan ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] async-tx: fix buffer sumbission error handling in ipu_idma.c 2010-02-10 15:54 ` Dan Williams @ 2010-02-10 16:32 ` Guennadi Liakhovetski 2010-02-10 17:47 ` Dan Williams 0 siblings, 1 reply; 7+ messages in thread From: Guennadi Liakhovetski @ 2010-02-10 16:32 UTC (permalink / raw) To: linux-arm-kernel If submitting new buffer failed, a wrong descriptor gets completed and it doesn't check, if a callback is at all defined, which can lead to an Oops. Fix these bugs and make ipu_update_channel_buffer() void, because it never fails. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- v1 -> v2: When fixing Oopses remember to not introduce compiler warnings;) On Wed, 10 Feb 2010, Dan Williams wrote: > On Wed, Feb 10, 2010 at 8:44 AM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > If submitting new buffer failed, a wrong descriptor gets completed and it > > doesn't check, if a callback is at all defined, which can lead to an Oops. Fix > > these bugs and make ipu_update_channel_buffer() void, because it never fails. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > Looks like 2.6.33 material. Shall I tag it for -stable as well? This is more of a theoretical Oops possibility. You have to really try hard to get it - if at all possible. At least I've never seen one and never heard anyone seeing it. So, strictly speaking, I don't think this bug can hurt anyone, but, if Sascha has nothing against this patch, you can push it to stable too. drivers/dma/ipu/ipu_idmac.c | 25 +++++++------------------ 1 files changed, 7 insertions(+), 18 deletions(-) diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c index 9a5bc1a..e80bae1 100644 --- a/drivers/dma/ipu/ipu_idmac.c +++ b/drivers/dma/ipu/ipu_idmac.c @@ -761,12 +761,10 @@ static void ipu_select_buffer(enum ipu_channel channel, int buffer_n) * @buffer_n: buffer number to update. * 0 or 1 are the only valid values. * @phyaddr: buffer physical address. - * @return: Returns 0 on success or negative error code on failure. This - * function will fail if the buffer is set to ready. */ /* Called under spin_lock(_irqsave)(&ichan->lock) */ -static int ipu_update_channel_buffer(struct idmac_channel *ichan, - int buffer_n, dma_addr_t phyaddr) +static void ipu_update_channel_buffer(struct idmac_channel *ichan, + int buffer_n, dma_addr_t phyaddr) { enum ipu_channel channel = ichan->dma_chan.chan_id; uint32_t reg; @@ -806,8 +804,6 @@ static int ipu_update_channel_buffer(struct idmac_channel *ichan, } spin_unlock_irqrestore(&ipu_data.lock, flags); - - return 0; } /* Called under spin_lock_irqsave(&ichan->lock) */ @@ -816,7 +812,6 @@ static int ipu_submit_buffer(struct idmac_channel *ichan, { unsigned int chan_id = ichan->dma_chan.chan_id; struct device *dev = &ichan->dma_chan.dev->device; - int ret; if (async_tx_test_ack(&desc->txd)) return -EINTR; @@ -827,14 +822,7 @@ static int ipu_submit_buffer(struct idmac_channel *ichan, * could make it conditional on status >= IPU_CHANNEL_ENABLED, but * doing it again shouldn't hurt either. */ - ret = ipu_update_channel_buffer(ichan, buf_idx, - sg_dma_address(sg)); - - if (ret < 0) { - dev_err(dev, "Updating sg %p on channel 0x%x buffer %d failed!\n", - sg, chan_id, buf_idx); - return ret; - } + ipu_update_channel_buffer(ichan, buf_idx, sg_dma_address(sg)); ipu_select_buffer(chan_id, buf_idx); dev_dbg(dev, "Updated sg %p on channel 0x%x buffer %d\n", @@ -1379,10 +1367,11 @@ static irqreturn_t idmac_interrupt(int irq, void *dev_id) if (likely(sgnew) && ipu_submit_buffer(ichan, descnew, sgnew, ichan->active_buffer) < 0) { - callback = desc->txd.callback; - callback_param = desc->txd.callback_param; + callback = descnew->txd.callback; + callback_param = descnew->txd.callback_param; spin_unlock(&ichan->lock); - callback(callback_param); + if (callback) + callback(callback_param); spin_lock(&ichan->lock); } -- 1.6.2.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2] async-tx: fix buffer sumbission error handling in ipu_idma.c 2010-02-10 16:32 ` [PATCH v2] " Guennadi Liakhovetski @ 2010-02-10 17:47 ` Dan Williams 2010-02-10 18:04 ` Uwe Kleine-König 0 siblings, 1 reply; 7+ messages in thread From: Dan Williams @ 2010-02-10 17:47 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 10, 2010 at 9:32 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > If submitting new buffer failed, a wrong descriptor gets completed and it > doesn't check, if a callback is at all defined, which can lead to an Oops. Fix > these bugs and make ipu_update_channel_buffer() void, because it never fails. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- > > v1 -> v2: When fixing Oopses remember to not introduce compiler warnings;) > > On Wed, 10 Feb 2010, Dan Williams wrote: > >> On Wed, Feb 10, 2010 at 8:44 AM, Guennadi Liakhovetski >> <g.liakhovetski@gmx.de> wrote: >> > If submitting new buffer failed, a wrong descriptor gets completed and it >> > doesn't check, if a callback is at all defined, which can lead to an Oops. Fix >> > these bugs and make ipu_update_channel_buffer() void, because it never fails. >> > >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> >> >> Looks like 2.6.33 material. ?Shall I tag it for -stable as well? > > This is more of a theoretical Oops possibility. You have to really try > hard to get it - if at all possible. At least I've never seen one and > never heard anyone seeing it. So, strictly speaking, I don't think this > bug can hurt anyone, but, if Sascha has nothing against this patch, you > can push it to stable too. Given the time proximity to 2.6.33-final I'll push it without the stable tag. If we later decide it is needed there it's just a matter of sending Greg a note. Thanks, Dan ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] async-tx: fix buffer sumbission error handling in ipu_idma.c 2010-02-10 17:47 ` Dan Williams @ 2010-02-10 18:04 ` Uwe Kleine-König 2010-02-10 20:11 ` Guennadi Liakhovetski 0 siblings, 1 reply; 7+ messages in thread From: Uwe Kleine-König @ 2010-02-10 18:04 UTC (permalink / raw) To: linux-arm-kernel Hello Guennadi, s/sumbission/submission/ in the Subject Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] async-tx: fix buffer sumbission error handling in ipu_idma.c 2010-02-10 18:04 ` Uwe Kleine-König @ 2010-02-10 20:11 ` Guennadi Liakhovetski 2010-02-10 20:37 ` Dan Williams 0 siblings, 1 reply; 7+ messages in thread From: Guennadi Liakhovetski @ 2010-02-10 20:11 UTC (permalink / raw) To: linux-arm-kernel On Wed, 10 Feb 2010, Uwe Kleine-K?nig wrote: > Hello Guennadi, > > s/sumbission/submission/ in the Subject Right, thanks Uwe. Dan, can you fix this when committing or shall I make v3? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] async-tx: fix buffer sumbission error handling in ipu_idma.c 2010-02-10 20:11 ` Guennadi Liakhovetski @ 2010-02-10 20:37 ` Dan Williams 0 siblings, 0 replies; 7+ messages in thread From: Dan Williams @ 2010-02-10 20:37 UTC (permalink / raw) To: linux-arm-kernel 2010/2/10 Guennadi Liakhovetski <g.liakhovetski@gmx.de>: > On Wed, 10 Feb 2010, Uwe Kleine-K?nig wrote: > >> Hello Guennadi, >> >> s/sumbission/submission/ in the Subject > > Right, thanks Uwe. Dan, can you fix this when committing or shall I make > v3? I applied it with this fix. -- Dan ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-02-10 20:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-10 15:44 [PATCH] async-tx: fix buffer sumbission error handling in ipu_idma.c Guennadi Liakhovetski 2010-02-10 15:54 ` Dan Williams 2010-02-10 16:32 ` [PATCH v2] " Guennadi Liakhovetski 2010-02-10 17:47 ` Dan Williams 2010-02-10 18:04 ` Uwe Kleine-König 2010-02-10 20:11 ` Guennadi Liakhovetski 2010-02-10 20:37 ` Dan Williams
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).