Re: [RESEND PATCH 1/1] staging:vt6655: remove checks around dev_kfree_skb

Re: [RESEND PATCH 1/1] staging:vt6655: remove checks around dev_kfree_skb

[Maninder Singh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 1594 bytes --]

Hi Dan,

>I hate these patches.  I have told Markus to stop sending them but he
>has issues so now I only complain when they introduce a bug.  There was
>one bug I have missed because it was a benchmark regression and I knew
>it was theoretically possible but I didn't know the code well enough to
>say which were fast paths...

>My main objection is that relying on the sanity check inside the
>function call makes the code more subtle to understand.  We know we need
>a NULL check but it is hidden away in another file.  The motivation for
>this patch you are sending is "There is a sanity check in dev_kfree_skb()
>so let's do an insane thing and save some lines of code."

>For this particular patch we assume throughout the whole driver that
>"pTDInfo->skb" can be NULL so making it inconsistent in this one place
>is wrong

Agreed,
But these changes are suggested because:-

where we are checking for (pTDInfo->skb), we are using it  in above line.
and it does not look good, thats why we should remove thse checks and i have suggested
changes.

code snippet:-
-----------------------

if (pTDInfo->skb_dma && (pTDInfo->skb_dma != pTDInfo->buf_dma))
    dma_unmap_single(&pDevice->pcid->dev, pTDInfo->skb_dma,
             pTDInfo->skb->len, DMA_TO_DEVICE);

----> In this we did not check for pTDInfo->skb

if (pTDInfo->skb)
    dev_kfree_skb(pTDInfo->skb);

But if am wrong, sorry for the patch.

Thanks,
Maninder
............ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RESEND PATCH 1/1] staging:vt6655: remove checks around dev_kfree_skb

[Dan Carpenter]

On Thu, Jul 16, 2015 at 08:45:06AM +0000, Maninder Singh wrote:
> where we are checking for (pTDInfo->skb), we are using it  in above line.
> and it does not look good, thats why we should remove thse checks and i have suggested
> changes.
> 
> code snippet:-
> -----------------------
> 
> if (pTDInfo->skb_dma && (pTDInfo->skb_dma != pTDInfo->buf_dma))
      ^^^^^^^^^^^^^^^^
This is allocated after ->skb so if ->skb_dma is non-NULL then ->skb is
also non-NULL.

>     dma_unmap_single(&pDevice->pcid->dev, pTDInfo->skb_dma,
>              pTDInfo->skb->len, DMA_TO_DEVICE);

Is this a static checker warning?  If so then it's a false positive.

regards,
dan carpenter

Re: [RESEND PATCH 1/1] staging:vt6655: remove checks around dev_kfree_skb

[Malcolm Priestley]

On 16/07/15 09:54, Dan Carpenter wrote:
> On Thu, Jul 16, 2015 at 08:45:06AM +0000, Maninder Singh wrote:
>> where we are checking for (pTDInfo->skb), we are using it  in above line.
>> and it does not look good, thats why we should remove thse checks and i have suggested
>> changes.
>>
>> code snippet:-
>> -----------------------
>>
>> if (pTDInfo->skb_dma && (pTDInfo->skb_dma != pTDInfo->buf_dma))
>        ^^^^^^^^^^^^^^^^
> This is allocated after ->skb so if ->skb_dma is non-NULL then ->skb is
> also non-NULL.
>
>>      dma_unmap_single(&pDevice->pcid->dev, pTDInfo->skb_dma,
>>               pTDInfo->skb->len, DMA_TO_DEVICE);
>
> Is this a static checker warning?  If so then it's a false positive.
>

This is old code dma_unmap_single never happens and is to be removed.

pTDInfo->skb now originates from mac80211.

I am in the process of tiding up pTDInfo that removes skb_dma.

In the past, this driver several API's sharing the same device now it 
has just one.

Regards


Malcolm

[RESEND PATCH 1/1] staging:vt6655: remove checks around dev_kfree_skb

[Maninder Singh]

dev_kfree_skb checks for NULL pointer itself,
Thus no need of explicit NULL check.

Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 drivers/staging/vt6655/device_main.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index ed040fb..1a50ea6 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -758,9 +758,7 @@ static void device_free_td0_ring(struct vnt_private *pDevice)
 			dma_unmap_single(&pDevice->pcid->dev, pTDInfo->skb_dma,
 					 pTDInfo->skb->len, DMA_TO_DEVICE);
 
-		if (pTDInfo->skb)
-			dev_kfree_skb(pTDInfo->skb);
-
+		dev_kfree_skb(pTDInfo->skb);
 		kfree(pDesc->pTDInfo);
 	}
 }
@@ -777,9 +775,7 @@ static void device_free_td1_ring(struct vnt_private *pDevice)
 			dma_unmap_single(&pDevice->pcid->dev, pTDInfo->skb_dma,
 					 pTDInfo->skb->len, DMA_TO_DEVICE);
 
-		if (pTDInfo->skb)
-			dev_kfree_skb(pTDInfo->skb);
-
+		dev_kfree_skb(pTDInfo->skb);
 		kfree(pDesc->pTDInfo);
 	}
 }
-- 
1.7.9.5

Re: [RESEND PATCH 1/1] staging:vt6655: remove checks around dev_kfree_skb

[Dan Carpenter]

On Wed, Jul 15, 2015 at 08:52:51AM +0530, Maninder Singh wrote:
> dev_kfree_skb checks for NULL pointer itself,
> Thus no need of explicit NULL check.
> 

I hate these patches.  I have told Markus to stop sending them but he
has issues so now I only complain when they introduce a bug.  There was
one bug I have missed because it was a benchmark regression and I knew
it was theoretically possible but I didn't know the code well enough to
say which were fast paths...

My main objection is that relying on the sanity check inside the
function call makes the code more subtle to understand.  We know we need
a NULL check but it is hidden away in another file.  The motivation for
this patch you are sending is "There is a sanity check in dev_kfree_skb()
so let's do an insane thing and save some lines of code."

For this particular patch we assume throughout the whole driver that
"pTDInfo->skb" can be NULL so making it inconsistent in this one place
is wrong.

regards,
dan carpenter