| Message ID | 1411994541-31494-1-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Sep 29, 2014 at 02:42:18PM +0200, Krzysztof Kozlowski wrote: > The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and > 'struct dma_pl330_desc desc' parameters for non-NULL. However these > checks are useless because supplied arguments won't be NULL. even if we have some error or bug? I would like this to be checked and complained loudly so we know something is going wrong rather than assuming it will be correct always.
On 10/14/2014 01:51 PM, Vinod Koul wrote: > On Mon, Sep 29, 2014 at 02:42:18PM +0200, Krzysztof Kozlowski wrote: >> The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and >> 'struct dma_pl330_desc desc' parameters for non-NULL. However these >> checks are useless because supplied arguments won't be NULL. > even if we have some error or bug? > > I would like this to be checked and complained loudly so we know something > is going wrong rather than assuming it will be correct always. > In this case it is impossible to get to this code portion if the pointer is NULL because it would have crashed earlier. - Lars -- 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 wto, 2014-10-14 at 17:21 +0530, Vinod Koul wrote: > On Mon, Sep 29, 2014 at 02:42:18PM +0200, Krzysztof Kozlowski wrote: > > The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and > > 'struct dma_pl330_desc desc' parameters for non-NULL. However these > > checks are useless because supplied arguments won't be NULL. > even if we have some error or bug? > > I would like this to be checked and complained loudly so we know something > is going wrong rather than assuming it will be correct always. Currently the driver would fail before or after, regardless of this check. However I do not insist on this approach. The first version of patch was little different: https://lkml.org/lkml/2014/9/5/390 Do you think that 1st version is worth resending? Best regards, Krzysztof -- 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, Oct 14, 2014 at 02:40:09PM +0200, Lars-Peter Clausen wrote: > On 10/14/2014 01:51 PM, Vinod Koul wrote: > >On Mon, Sep 29, 2014 at 02:42:18PM +0200, Krzysztof Kozlowski wrote: > >>The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and > >>'struct dma_pl330_desc desc' parameters for non-NULL. However these > >>checks are useless because supplied arguments won't be NULL. > >even if we have some error or bug? > > > >I would like this to be checked and complained loudly so we know something > >is going wrong rather than assuming it will be correct always. > > > > In this case it is impossible to get to this code portion if the > pointer is NULL because it would have crashed earlier. Yes checking again, for desc it seems not neccessary as its deref earlier in calling code and thrd shouldnt be NULL, so will apply now
================= 1. Remove the checks for non-NULL completely instead of moving them before dereference. Suggested by Lars-Peter Clausen. --- drivers/dma/pl330.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index d5149aacd2fe..57049f84d0c0 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -1372,10 +1372,6 @@ static int pl330_submit_req(struct pl330_thread *thrd, u32 ccr; int ret = 0; - /* No Req or Unacquired Channel or DMAC */ - if (!desc || !thrd || thrd->free) - return -EINVAL; - regs = thrd->dmac->base; if (pl330->state == DYING
The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and 'struct dma_pl330_desc desc' parameters for non-NULL. However these checks are useless because supplied arguments won't be NULL. The pl330_submit_req() is called in only one place and: 1. 'desc' is already dereferenced in fill_queue() before calling pl330_submit_req(). 2. 'thrd' is always dereferenced after calling fill_queue()->pl330_submit_req(). Removing the checks for non-NULL values fixes following warning: drivers/dma/pl330.c:1376 pl330_submit_req() warn: variable dereferenced before check 'thrd' (see line 1367) Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- Changes since v1: