Re: [PATCH] net: wireless: prefix header search paths with $(srctree)/

[Hin-Tak Leung]

On Fri, 25/1/19, Masahiro Yamada  wrote:
 
> Currently, the Kbuild core manipulates header
> search paths in a crazy
> way [1].
 
> To fix this mess, I want all Makefiles
> to add explicit $(srctree)/ to
> the search paths in the srctree. Some
> Makefiles are already written in
> that way, but not all. The goal of this
> work is to make the notation
> consistent, and finally get rid of the
> gross hacks.
 
> Having whitespaces after -I does not
> matter since commit 48f6e3cf5bc6
> ("kbuild: do not drop -I without
> parameter").
 
> I also removed one header search path
> in:
 
  
> drivers/net/wireless/broadcom/brcm80211/brcmutil/Makefile
 
> I was able to compile without it.
 
> [1]: https://patchwork.kernel.org/patch/9632347/
 
> Signed-off-by: Masahiro Yamada 

Acked-by: Hin-Tak Leung 

Looks okay for the rtl818x parts.

Re: [PATCH] rtl8187: Fix NULL pointer dereference in priv->conf_mutex

[Hin-Tak Leung]

On Thu, 15/2/18, Sudhir Sreedharan <ssreedha...@mvista.com> wrote:

...
 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Sudhir Sreedharan <ssreedha...@mvista.com>

Acked-by: Hin-Tak Leung <ht...@users.sourceforge.net>

Re: [PATCH] rtl8187: Fix NULL pointer dereference in priv->conf_mutex

[Hin-Tak Leung]

On Thu, 15/2/18, Sudhir Sreedharan  wrote:

...
 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Sudhir Sreedharan 

Acked-by: Hin-Tak Leung 

Re: [PATCH] rtlwifi: rtl818x: remove redundant check for cck_power > 15

[Hin-Tak Leung]

On Tue, 14/11/17, Colin King <colin.k...@canonical.com> wrote:

 
> From: Colin Ian King <colin.k...@canonical.com>
 
> cck_poweri cannot be greated than 15 as
> is derived from the bottom 4 bits
> from riv->channels[channel -
> 1].hw_value & 0xf.  Hence the check for it
> being greater than 15 is redundant and
> can be removed.
 
> Detected by CoverityScan, CID#744303
> ("Logically dead code")
 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
 
Acked-by: Hin-Tak Leung <ht...@users.sourceforge.net>

Re: [PATCH] rtlwifi: rtl818x: remove redundant check for cck_power > 15

[Hin-Tak Leung]

On Tue, 14/11/17, Colin King  wrote:

 
> From: Colin Ian King 
 
> cck_poweri cannot be greated than 15 as
> is derived from the bottom 4 bits
> from riv->channels[channel -
> 1].hw_value & 0xf.  Hence the check for it
> being greater than 15 is redundant and
> can be removed.
 
> Detected by CoverityScan, CID#744303
> ("Logically dead code")
 
> Signed-off-by: Colin Ian King 
 
Acked-by: Hin-Tak Leung 

Re: [PATCH 31/35] wireless: realtek: rtl8187: constify usb_device_id

[Hin-Tak Leung]

On Tue, 8/8/17, Arvind Yadav  wrote:

 Subject: [PATCH 31/35] wireless: realtek: rtl8187: constify usb_device_id
 To: kv...@codeaurora.org, her...@canonical.com, ht...@users.sourceforge.net, 
larry.fin...@lwfinger.net
 Cc: linux-kernel@vger.kernel.org, net...@vger.kernel.org, 
linux-wirel...@vger.kernel.org
 Date: Tuesday, 8 August, 2017, 17:04
 
> usb_device_id are not supposed to change at
> runtime. All functions
> working with usb_device_id provided by
>  work with
> const usb_device_id. So mark the
> non-const structs as const.
 
> Signed-off-by: Arvind Yadav 
 
Acked-by: ht...@users.sourceforge.net

Re: [PATCH 31/35] wireless: realtek: rtl8187: constify usb_device_id

[Hin-Tak Leung]

On Tue, 8/8/17, Arvind Yadav  wrote:

 Subject: [PATCH 31/35] wireless: realtek: rtl8187: constify usb_device_id
 To: kv...@codeaurora.org, her...@canonical.com, ht...@users.sourceforge.net, 
larry.fin...@lwfinger.net
 Cc: linux-kernel@vger.kernel.org, net...@vger.kernel.org, 
linux-wirel...@vger.kernel.org
 Date: Tuesday, 8 August, 2017, 17:04
 
> usb_device_id are not supposed to change at
> runtime. All functions
> working with usb_device_id provided by
>  work with
> const usb_device_id. So mark the
> non-const structs as const.
 
> Signed-off-by: Arvind Yadav 
 
Acked-by: ht...@users.sourceforge.net

Re: [PATCH] rtl8187: fix use after free on failure path in rtl8187_probe()

[Hin-Tak Leung]

--
On Sun, Mar 30, 2014 10:31 PM BST Larry Finger wrote:

>On 03/28/2014 03:26 PM, Alexey Khoroshilov wrote:
> If allocation of io_dmabuf fails, rtl8187_probe() calls usb_put_dev(udev)
> while usb_get_dev(udev) is not called yet. As a result refcnt is decremented
> incorrectly and usb_dev can be used after memory deallocation.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov 
> ---
>
>Acked-by: Larry Finger 
>
>Thanks,
>
>Larry

Acked-by: Hin-Tak Leung 

Hin-Tak

>
>   drivers/net/wireless/rtl818x/rtl8187/dev.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c 
> b/drivers/net/wireless/rtl818x/rtl8187/dev.c
> index fd78df813a85..d7f540a9dc9b 100644
> --- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
> +++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
> @@ -1636,10 +1636,10 @@ static int rtl8187_probe(struct usb_interface *intf,
>
>    err_free_dmabuf:
>   kfree(priv->io_dmabuf);
> - err_free_dev:
> -    ieee80211_free_hw(dev);
>   usb_set_intfdata(intf, NULL);
>   usb_put_dev(udev);
> + err_free_dev:
> +    ieee80211_free_hw(dev);
>   return err;
>   }
>
>
>


--
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] rtl8187: fix use after free on failure path in rtl8187_probe()

[Hin-Tak Leung]

--
On Sun, Mar 30, 2014 10:31 PM BST Larry Finger wrote:

On 03/28/2014 03:26 PM, Alexey Khoroshilov wrote:
 If allocation of io_dmabuf fails, rtl8187_probe() calls usb_put_dev(udev)
 while usb_get_dev(udev) is not called yet. As a result refcnt is decremented
 incorrectly and usb_dev can be used after memory deallocation.

 Found by Linux Driver Verification project (linuxtesting.org).

 Signed-off-by: Alexey Khoroshilov khoroshi...@ispras.ru
 ---

Acked-by: Larry Finger larry.fin...@lwfinger.net

Thanks,

Larry

Acked-by: Hin-Tak Leung ht...@users.sourceforge.net

Hin-Tak


   drivers/net/wireless/rtl818x/rtl8187/dev.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c 
 b/drivers/net/wireless/rtl818x/rtl8187/dev.c
 index fd78df813a85..d7f540a9dc9b 100644
 --- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
 +++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
 @@ -1636,10 +1636,10 @@ static int rtl8187_probe(struct usb_interface *intf,

    err_free_dmabuf:
   kfree(priv-io_dmabuf);
 - err_free_dev:
 -    ieee80211_free_hw(dev);
   usb_set_intfdata(intf, NULL);
   usb_put_dev(udev);
 + err_free_dev:
 +    ieee80211_free_hw(dev);
   return err;
   }





--
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] rtl8187: fix use after free on failure path in rtl8187_init_urbs()

[Hin-Tak Leung]

--
On Mon, Sep 2, 2013 05:06 BST Alexey Khoroshilov wrote:

>On 01.09.2013 10:51, Hin-Tak Leung wrote:
>> --
>> On Sat, Aug 31, 2013 22:18 BST Alexey Khoroshilov wrote:
>>
>> In case of __dev_alloc_skb() failure rtl8187_init_urbs()
>> calls usb_free_urb(entry) where 'entry' can points to urb
>> allocated at the previous iteration. That means refcnt will be
>> decremented incorrectly and the urb can be used after memory
>> deallocation.
>>
>> The patch fixes the issue and implements error handling of init_urbs
>> in rtl8187_start().
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> Signed-off-by: Alexey Khoroshilov 
>> ---
>> drivers/net/wireless/rtl818x/rtl8187/dev.c | 15 ++-
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c 
>> b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> index f49220e..e83d53c 100644
>> --- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> +++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> @@ -438,17 +438,16 @@ static int rtl8187_init_urbs(struct ieee80211_hw *dev)
>>          skb_queue_tail(>rx_queue, skb);
>>          usb_anchor_urb(entry, >anchored);
>>          ret = usb_submit_urb(entry, GFP_KERNEL);
>> +        usb_free_urb(entry);
>>          if (ret) {
>>              skb_unlink(skb, >rx_queue);
>>              usb_unanchor_urb(entry);
>>              goto err;
>>          }
>> -        usb_free_urb(entry);
>>      }
>>      return ret;
>>
>> err:
>> -    usb_free_urb(entry);
>>      kfree_skb(skb);
>>      usb_kill_anchored_urbs(>anchored);
>>      return ret;
>> This part looks wrong - you free_urb(entry) then unanchor_urb(entry).
>I do not see any problems here.
>usb_free_urb() just decrements refcnt of the urb.
>While usb_anchor_urb() and usb_unanchor_urb() increment and decrement it 
>as well.
>So actual memory deallocation will happen in usb_unanchor_urb().

If the routines work as you say, they probably are misnamed, and/or prototyped 
wrongly?
Also, you are making assumptions about how they are implemented, and relying
on the implementation details to be fixed for eternity.

I am just saying,

XXX_free(some_entity);
if(condtion)
 do_stuff(some_entity);

looks wrong, and if that's intentional, those routines really shouldn't be 
named as such.

Hin-Tak



--
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] rtl8187: fix use after free on failure path in rtl8187_init_urbs()

[Hin-Tak Leung]

--
On Mon, Sep 2, 2013 05:06 BST Alexey Khoroshilov wrote:

On 01.09.2013 10:51, Hin-Tak Leung wrote:
 --
 On Sat, Aug 31, 2013 22:18 BST Alexey Khoroshilov wrote:

 In case of __dev_alloc_skb() failure rtl8187_init_urbs()
 calls usb_free_urb(entry) where 'entry' can points to urb
 allocated at the previous iteration. That means refcnt will be
 decremented incorrectly and the urb can be used after memory
 deallocation.

 The patch fixes the issue and implements error handling of init_urbs
 in rtl8187_start().

 Found by Linux Driver Verification project (linuxtesting.org).

 Signed-off-by: Alexey Khoroshilov khoroshi...@ispras.ru
 ---
 drivers/net/wireless/rtl818x/rtl8187/dev.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

 diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c 
 b/drivers/net/wireless/rtl818x/rtl8187/dev.c
 index f49220e..e83d53c 100644
 --- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
 +++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
 @@ -438,17 +438,16 @@ static int rtl8187_init_urbs(struct ieee80211_hw *dev)
          skb_queue_tail(priv-rx_queue, skb);
          usb_anchor_urb(entry, priv-anchored);
          ret = usb_submit_urb(entry, GFP_KERNEL);
 +        usb_free_urb(entry);
          if (ret) {
              skb_unlink(skb, priv-rx_queue);
              usb_unanchor_urb(entry);
              goto err;
          }
 -        usb_free_urb(entry);
      }
      return ret;

 err:
 -    usb_free_urb(entry);
      kfree_skb(skb);
      usb_kill_anchored_urbs(priv-anchored);
      return ret;
 This part looks wrong - you free_urb(entry) then unanchor_urb(entry).
I do not see any problems here.
usb_free_urb() just decrements refcnt of the urb.
While usb_anchor_urb() and usb_unanchor_urb() increment and decrement it 
as well.
So actual memory deallocation will happen in usb_unanchor_urb().

If the routines work as you say, they probably are misnamed, and/or prototyped 
wrongly?
Also, you are making assumptions about how they are implemented, and relying
on the implementation details to be fixed for eternity.

I am just saying,

XXX_free(some_entity);
if(condtion)
 do_stuff(some_entity);

looks wrong, and if that's intentional, those routines really shouldn't be 
named as such.

Hin-Tak



--
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] rtl8187: fix use after free on failure path in rtl8187_init_urbs()

[Hin-Tak Leung]

--
On Sat, Aug 31, 2013 22:18 BST Alexey Khoroshilov wrote:

>In case of __dev_alloc_skb() failure rtl8187_init_urbs()
>calls usb_free_urb(entry) where 'entry' can points to urb
>allocated at the previous iteration. That means refcnt will be
>decremented incorrectly and the urb can be used after memory
>deallocation.
>
>The patch fixes the issue and implements error handling of init_urbs
>in rtl8187_start().
>
>Found by Linux Driver Verification project (linuxtesting.org).
>
>Signed-off-by: Alexey Khoroshilov 
>---
> drivers/net/wireless/rtl818x/rtl8187/dev.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c 
>b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>index f49220e..e83d53c 100644
>--- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
>+++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>@@ -438,17 +438,16 @@ static int rtl8187_init_urbs(struct ieee80211_hw *dev)
>         skb_queue_tail(>rx_queue, skb);
>         usb_anchor_urb(entry, >anchored);
>         ret = usb_submit_urb(entry, GFP_KERNEL);
>+        usb_free_urb(entry);
>         if (ret) {
>             skb_unlink(skb, >rx_queue);
>             usb_unanchor_urb(entry);
>             goto err;
>         }
>-        usb_free_urb(entry);
>     }
>     return ret;
>
> err:
>-    usb_free_urb(entry);
>     kfree_skb(skb);
>     usb_kill_anchored_urbs(>anchored);
>     return ret;

This part looks wrong - you free_urb(entry) then unanchor_urb(entry).

>@@ -956,8 +955,12 @@ static int rtl8187_start(struct ieee80211_hw *dev)
>                   (RETRY_COUNT < 8  /* short retry limit */) |
>                   (RETRY_COUNT < 0  /* long retry limit */) |
>                   (7 < 21 /* MAX TX DMA */));
>-        rtl8187_init_urbs(dev);
>-        rtl8187b_init_status_urb(dev);
>+        ret = rtl8187_init_urbs(dev);
>+        if (ret)
>+            goto rtl8187_start_exit;
>+        ret = rtl8187b_init_status_urb(dev);
>+        if (ret)
>+            usb_kill_anchored_urbs(>anchored);
>         goto rtl8187_start_exit;
>     }
> 
>@@ -966,7 +969,9 @@ static int rtl8187_start(struct ieee80211_hw *dev)
>     rtl818x_iowrite32(priv, >map->MAR[0], ~0);
>     rtl818x_iowrite32(priv, >map->MAR[1], ~0);
> 
>-    rtl8187_init_urbs(dev);
>+    ret = rtl8187_init_urbs(dev);
>+    if (ret)
>+        goto rtl8187_start_exit;
> 
>     reg = RTL818X_RX_CONF_ONLYERLPKT |
>           RTL818X_RX_CONF_RX_AUTORESETPHY |
>-- 
>1.8.1.2
>


--
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] rtl8187: fix use after free on failure path in rtl8187_init_urbs()

[Hin-Tak Leung]

--
On Sat, Aug 31, 2013 22:18 BST Alexey Khoroshilov wrote:

In case of __dev_alloc_skb() failure rtl8187_init_urbs()
calls usb_free_urb(entry) where 'entry' can points to urb
allocated at the previous iteration. That means refcnt will be
decremented incorrectly and the urb can be used after memory
deallocation.

The patch fixes the issue and implements error handling of init_urbs
in rtl8187_start().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov khoroshi...@ispras.ru
---
 drivers/net/wireless/rtl818x/rtl8187/dev.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c 
b/drivers/net/wireless/rtl818x/rtl8187/dev.c
index f49220e..e83d53c 100644
--- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
@@ -438,17 +438,16 @@ static int rtl8187_init_urbs(struct ieee80211_hw *dev)
         skb_queue_tail(priv-rx_queue, skb);
         usb_anchor_urb(entry, priv-anchored);
         ret = usb_submit_urb(entry, GFP_KERNEL);
+        usb_free_urb(entry);
         if (ret) {
             skb_unlink(skb, priv-rx_queue);
             usb_unanchor_urb(entry);
             goto err;
         }
-        usb_free_urb(entry);
     }
     return ret;

 err:
-    usb_free_urb(entry);
     kfree_skb(skb);
     usb_kill_anchored_urbs(priv-anchored);
     return ret;

This part looks wrong - you free_urb(entry) then unanchor_urb(entry).

@@ -956,8 +955,12 @@ static int rtl8187_start(struct ieee80211_hw *dev)
                   (RETRY_COUNT  8  /* short retry limit */) |
                   (RETRY_COUNT  0  /* long retry limit */) |
                   (7  21 /* MAX TX DMA */));
-        rtl8187_init_urbs(dev);
-        rtl8187b_init_status_urb(dev);
+        ret = rtl8187_init_urbs(dev);
+        if (ret)
+            goto rtl8187_start_exit;
+        ret = rtl8187b_init_status_urb(dev);
+        if (ret)
+            usb_kill_anchored_urbs(priv-anchored);
         goto rtl8187_start_exit;
     }
 
@@ -966,7 +969,9 @@ static int rtl8187_start(struct ieee80211_hw *dev)
     rtl818x_iowrite32(priv, priv-map-MAR[0], ~0);
     rtl818x_iowrite32(priv, priv-map-MAR[1], ~0);
 
-    rtl8187_init_urbs(dev);
+    ret = rtl8187_init_urbs(dev);
+    if (ret)
+        goto rtl8187_start_exit;
 
     reg = RTL818X_RX_CONF_ONLYERLPKT |
           RTL818X_RX_CONF_RX_AUTORESETPHY |
-- 
1.8.1.2



--
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 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

[Hin-Tak Leung]

--- On Mon, 15/4/13, Joe Perches  wrote:

> On Mon, 2013-04-15 at 04:46 +0100,
> Hin-Tak Leung wrote:
> > > By converting this dprint() to pr_debug(), it
> would
> > > print out on a multiple lines, one for each read.
> > > 
> > > That's why it should use a mechanism like
> dbg_cont.
> > > 
> > > btw: there is no current pr_debug_cont mechanism.
> > 
> > That's rubbish.
> 
> Don't be silly.
> 
> > dprint() are compiled in/out debug printing
> statements,
> > and are entirely suppressed in unmodified kernel
> source
> 
> Of course.
> 
> > I am not saying what I have in private is correct
> 
> Then your original post wasn't useful either.
> 
> > What I am saying is that the code snipplet I posted is
> functional:
> 
> Lots of code is functional, I prefer functional
> and correct though.
> 
> cheers, Joe

Hmm, you obvious has a different meaning of "functional" than I. How is 
converting a few hundred lines of "print nothing" to another few hundred lines 
of "print nothing" functional? What does it achieve?

I have already voiced my (admittedly selfish) concern: changing a few hundred 
lines of "print nothing" to another few hundred lines of "print nothing" means 
some of us who have substantial work-in-progress patches needs to spend a fair 
amount of time on rebase, and manually resolving conflicts from rebase.

--
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 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

[Hin-Tak Leung]

--- On Mon, 15/4/13, Joe Perches  wrote:

> On Mon, 2013-04-15 at 02:56 +0100,
> Hin-Tak Leung wrote:
> > --- On Mon, 15/4/13, Joe Perches 
> wrote:
> > > On Mon, 2013-04-15 at 01:53 +0100,
> > > Hin-Tak Leung wrote:
> > > > --- On Mon, 8/4/13, Joe Perches 
> wrote:
> > > > > Use a more current logging style.
> > > []
> > > > I have been sitting on a patch which changes
> this part
> > > of the code to dynamic debugging, and it is much
> simplier.
> []
> > > This change wouldn't work well as it would make a
> mess
> > > of output that uses no prefix (ie: emits at
> KERN_DEFAULT)
> > > with output that uses KERN_DEBUG
> > > 
> > > That's the reason for _dbg and _dbg_cont.
> > 
> > Hmm, I don't get it. Is there any *existing* use of
> dprint
> > in the hfplus code which is affected by your comment?
> 
> Code like this prints out currently on a single line at
> KERN_DEFAULT.
> 
> @@ -138,16 +138,16 @@ void hfs_bnode_dump(struct hfs_bnode
> *node)
> []
>         for (i =
> be16_to_cpu(desc.num_recs); i >= 0; off -= 2, i--) {
>                
> key_off = hfs_bnode_read_u16(node, off);
> -           
>    dprint(DBG_BNODE_MOD, " %d", key_off);
> +           
>    hfs_dbg_cont(BNODE_MOD, " %d", key_off);
> 
> By converting this dprint() to pr_debug(), it would
> print out on a multiple lines, one for each read.
> 
> That's why it should use a mechanism like dbg_cont.
> 
> btw: there is no current pr_debug_cont mechanism.

That's rubbish. dprint() are compiled in/out debug printing statements, and are 
entirely suppressed in unmodified kernel source (the value of DBG_MASK being 
zero). So your rather large and invasive change - which is still conditional on 
DBG_MASK - is just substituting one form of print nothing to another form of 
print nothing.

I am not saying what I have in private is correct - otherwise I would have 
submitted it a long time ago. What I am saying is that the code snipplet I 
posted is functional: it is not conditional on DBG_MASK, but conditional on the 
meaning of pr_debug (and only on it), which is either printing 
indiscriminantly, or on/off switchable at runtime for dynamically enabled 
kernel. And it is a small and non-invasive change in any case, which I can hang 
on to indefinitely.

I think the current *implementation* of dprint is bad - it depending on a 
modification of kernel source and re-compilation to make debug statement 
visible instead of the default "print nothing". But your patch, which modifies 
a lot of "print nothing" to another style of "print nothing", has no functional 
consequence at all. There is no user-visible change. It changes a few hundred 
lines of print nothing to another few hundred lines of print nothing.



 
--
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 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

[Hin-Tak Leung]

--- On Mon, 15/4/13, Joe Perches  wrote:

> On Mon, 2013-04-15 at 01:53 +0100,
> Hin-Tak Leung wrote:
> > --- On Mon, 8/4/13, Joe Perches 
> wrote:
> > > Use a more current logging style.
> []
> > I have been sitting on a patch which changes this part
> of the code to dynamic debugging, and it is much simplier.
> Just:
> > #define dprint(flg, fmt, args...) \
> > -       if (flg &
> DBG_MASK) \
> > -           
>    printk(fmt , ## args)
> > +           
>    pr_debug(fmt , ## args)
> 
> This change wouldn't work well as it would make a mess
> of output that uses no prefix (ie: emits at KERN_DEFAULT)
> with output that uses KERN_DEBUG
> 
> That's the reason for _dbg and _dbg_cont.

Hmm, I don't get it. Is there any *existing* use of dprint in the hfplus code 
which is affected by your comment? Or is this another general stylistic 
comment? i.e. "this does not work in general"?


--
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 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

[Hin-Tak Leung]

--- On Mon, 8/4/13, Joe Perches  wrote:

> Use a more current logging style.
> 
> Rename macro and uses.
> Add do {} while (0) to macro.
> Add DBG_ to macro.
> Add and use hfs_dbg_cont variant where appropriate.
> 
> Signed-off-by: Joe Perches 



> +++ b/fs/hfs/hfs_fs.h
> @@ -34,8 +34,18 @@
>  //#define DBG_MASK   
> (DBG_CAT_MOD|DBG_BNODE_REFS|DBG_INODE|DBG_EXTENT)
>  #define DBG_MASK    (0)
>  
> -#define dprint(flg, fmt, args...) \
> -    if (flg & DBG_MASK) printk(fmt , ##
> args)
> +#define hfs_dbg(flg, fmt, ...)   
>             \
> +do {       
>            
>         \
> +    if (DBG_##flg &
> DBG_MASK)       
>     \
> +        printk(KERN_DEBUG
> fmt, ##__VA_ARGS__);    \
> +} while (0)
> +
> +#define hfs_dbg_cont(flg, fmt, ...)   
>         \
> +do {       
>            
>         \
> +    if (DBG_##flg &
> DBG_MASK)       
>     \
> +        printk(KERN_CONT fmt,
> ##__VA_ARGS__);    \
> +} while (0)
> +
>  
>  /*
>   * struct hfs_inode_info



This set of change seems to be somewhat zealous - it doesn't offer any benefits 
other than possibly satisfying somebody's idea of code-purity.

FWIW, I have been sitting on a patch which changes this part of the code to 
dynamic debugging, and it is much simplier. Just:

=
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index e298b83..55d211d 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -45,8 +25,7 @@
 #define HFSPLUS_JOURNAL_SWAP   1
 
 #define dprint(flg, fmt, args...) \
-   if (flg & DBG_MASK) \
-   printk(fmt , ## args)
+   pr_debug(fmt , ## args)
 
 /* Runtime config options */
 #define HFSPLUS_DEF_CR_TYPE0x3F3F3F3F  /* '' */
=

(and you can then remove all the DBG_* defines before that, since they then 
don't have any effect any more).

The benefit of this alternative is that it does not break any out-of-tree 
patches, while make it easier to debug say patches... and I am still sitting on 
a rather substantial set of the journal change, plus all the other issues that 
come out of it, like the folder count patch for case-sensitive file systems.

I think one needs to think very carefully about make bulk changes like this, 
which serves no real purpose other than satisfying somebody's idea of code 
purity.

The problem with such bulk "stylistic" changes, is that it forces people who 
are working on real functionalities and bug fixes to rebase their work, and 
spend time on doing so, and also at the risk introducing new bugs while 
rebasing. I know I am writing on a somewhat selfish purpose: if I need to 
rebase my work due to other's bug fixes or enhancement, etc, then fair enough, 
but I'd prefer not to rebase for the purpose of other's preference of, and 
attempts at re-arranging the style of the debug statements, when the debugging 
output means little to them.





--
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 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

[Hin-Tak Leung]

--- On Mon, 15/4/13, Joe Perches j...@perches.com wrote:

 On Mon, 2013-04-15 at 01:53 +0100,
 Hin-Tak Leung wrote:
  --- On Mon, 8/4/13, Joe Perches j...@perches.com
 wrote:
   Use a more current logging style.
 []
  I have been sitting on a patch which changes this part
 of the code to dynamic debugging, and it is much simplier.
 Just:
  #define dprint(flg, fmt, args...) \
  -       if (flg 
 DBG_MASK) \
  -           
    printk(fmt , ## args)
  +           
    pr_debug(fmt , ## args)
 
 This change wouldn't work well as it would make a mess
 of output that uses no prefix (ie: emits at KERN_DEFAULT)
 with output that uses KERN_DEBUG
 
 That's the reason for _dbg and _dbg_cont.

Hmm, I don't get it. Is there any *existing* use of dprint in the hfplus code 
which is affected by your comment? Or is this another general stylistic 
comment? i.e. this does not work in general?


--
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 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

[Hin-Tak Leung]

--- On Mon, 15/4/13, Joe Perches j...@perches.com wrote:

 On Mon, 2013-04-15 at 02:56 +0100,
 Hin-Tak Leung wrote:
  --- On Mon, 15/4/13, Joe Perches j...@perches.com
 wrote:
   On Mon, 2013-04-15 at 01:53 +0100,
   Hin-Tak Leung wrote:
--- On Mon, 8/4/13, Joe Perches j...@perches.com
 wrote:
 Use a more current logging style.
   []
I have been sitting on a patch which changes
 this part
   of the code to dynamic debugging, and it is much
 simplier.
 []
   This change wouldn't work well as it would make a
 mess
   of output that uses no prefix (ie: emits at
 KERN_DEFAULT)
   with output that uses KERN_DEBUG
   
   That's the reason for _dbg and _dbg_cont.
  
  Hmm, I don't get it. Is there any *existing* use of
 dprint
  in the hfplus code which is affected by your comment?
 
 Code like this prints out currently on a single line at
 KERN_DEFAULT.
 
 @@ -138,16 +138,16 @@ void hfs_bnode_dump(struct hfs_bnode
 *node)
 []
         for (i =
 be16_to_cpu(desc.num_recs); i = 0; off -= 2, i--) {
                
 key_off = hfs_bnode_read_u16(node, off);
 -           
    dprint(DBG_BNODE_MOD,  %d, key_off);
 +           
    hfs_dbg_cont(BNODE_MOD,  %d, key_off);
 
 By converting this dprint() to pr_debug(), it would
 print out on a multiple lines, one for each read.
 
 That's why it should use a mechanism like dbg_cont.
 
 btw: there is no current pr_debug_cont mechanism.

That's rubbish. dprint() are compiled in/out debug printing statements, and are 
entirely suppressed in unmodified kernel source (the value of DBG_MASK being 
zero). So your rather large and invasive change - which is still conditional on 
DBG_MASK - is just substituting one form of print nothing to another form of 
print nothing.

I am not saying what I have in private is correct - otherwise I would have 
submitted it a long time ago. What I am saying is that the code snipplet I 
posted is functional: it is not conditional on DBG_MASK, but conditional on the 
meaning of pr_debug (and only on it), which is either printing 
indiscriminantly, or on/off switchable at runtime for dynamically enabled 
kernel. And it is a small and non-invasive change in any case, which I can hang 
on to indefinitely.

I think the current *implementation* of dprint is bad - it depending on a 
modification of kernel source and re-compilation to make debug statement 
visible instead of the default print nothing. But your patch, which modifies 
a lot of print nothing to another style of print nothing, has no functional 
consequence at all. There is no user-visible change. It changes a few hundred 
lines of print nothing to another few hundred lines of print nothing.



 
--
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 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

[Hin-Tak Leung]

--- On Mon, 15/4/13, Joe Perches j...@perches.com wrote:

 On Mon, 2013-04-15 at 04:46 +0100,
 Hin-Tak Leung wrote:
   By converting this dprint() to pr_debug(), it
 would
   print out on a multiple lines, one for each read.
   
   That's why it should use a mechanism like
 dbg_cont.
   
   btw: there is no current pr_debug_cont mechanism.
  
  That's rubbish.
 
 Don't be silly.
 
  dprint() are compiled in/out debug printing
 statements,
  and are entirely suppressed in unmodified kernel
 source
 
 Of course.
 
  I am not saying what I have in private is correct
 
 Then your original post wasn't useful either.
 
  What I am saying is that the code snipplet I posted is
 functional:
 
 Lots of code is functional, I prefer functional
 and correct though.
 
 cheers, Joe

Hmm, you obvious has a different meaning of functional than I. How is 
converting a few hundred lines of print nothing to another few hundred lines 
of print nothing functional? What does it achieve?

I have already voiced my (admittedly selfish) concern: changing a few hundred 
lines of print nothing to another few hundred lines of print nothing means 
some of us who have substantial work-in-progress patches needs to spend a fair 
amount of time on rebase, and manually resolving conflicts from rebase.

--
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 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

[Hin-Tak Leung]

--- On Mon, 8/4/13, Joe Perches j...@perches.com wrote:

 Use a more current logging style.
 
 Rename macro and uses.
 Add do {} while (0) to macro.
 Add DBG_ to macro.
 Add and use hfs_dbg_cont variant where appropriate.
 
 Signed-off-by: Joe Perches j...@perches.com

a lot of dprint to hfs_dbg changes snipped

 +++ b/fs/hfs/hfs_fs.h
 @@ -34,8 +34,18 @@
  //#define DBG_MASK   
 (DBG_CAT_MOD|DBG_BNODE_REFS|DBG_INODE|DBG_EXTENT)
  #define DBG_MASK    (0)
  
 -#define dprint(flg, fmt, args...) \
 -    if (flg  DBG_MASK) printk(fmt , ##
 args)
 +#define hfs_dbg(flg, fmt, ...)   
             \
 +do {       
            
         \
 +    if (DBG_##flg 
 DBG_MASK)       
     \
 +        printk(KERN_DEBUG
 fmt, ##__VA_ARGS__);    \
 +} while (0)
 +
 +#define hfs_dbg_cont(flg, fmt, ...)   
         \
 +do {       
            
         \
 +    if (DBG_##flg 
 DBG_MASK)       
     \
 +        printk(KERN_CONT fmt,
 ##__VA_ARGS__);    \
 +} while (0)
 +
  
  /*
   * struct hfs_inode_info

a lot of dprint to hfs_dbg change snipped

This set of change seems to be somewhat zealous - it doesn't offer any benefits 
other than possibly satisfying somebody's idea of code-purity.

FWIW, I have been sitting on a patch which changes this part of the code to 
dynamic debugging, and it is much simplier. Just:

=
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index e298b83..55d211d 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -45,8 +25,7 @@
 #define HFSPLUS_JOURNAL_SWAP   1
 
 #define dprint(flg, fmt, args...) \
-   if (flg  DBG_MASK) \
-   printk(fmt , ## args)
+   pr_debug(fmt , ## args)
 
 /* Runtime config options */
 #define HFSPLUS_DEF_CR_TYPE0x3F3F3F3F  /* '' */
=

(and you can then remove all the DBG_* defines before that, since they then 
don't have any effect any more).

The benefit of this alternative is that it does not break any out-of-tree 
patches, while make it easier to debug say patches... and I am still sitting on 
a rather substantial set of the journal change, plus all the other issues that 
come out of it, like the folder count patch for case-sensitive file systems.

I think one needs to think very carefully about make bulk changes like this, 
which serves no real purpose other than satisfying somebody's idea of code 
purity.

The problem with such bulk stylistic changes, is that it forces people who 
are working on real functionalities and bug fixes to rebase their work, and 
spend time on doing so, and also at the risk introducing new bugs while 
rebasing. I know I am writing on a somewhat selfish purpose: if I need to 
rebase my work due to other's bug fixes or enhancement, etc, then fair enough, 
but I'd prefer not to rebase for the purpose of other's preference of, and 
attempts at re-arranging the style of the debug statements, when the debugging 
output means little to them.





--
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] hfs: add error checking for hfs_find_init()

[Hin-Tak Leung]

--- On Sat, 30/3/13, Vyacheslav Dubeyko  wrote:

> From: Vyacheslav Dubeyko 
> Subject: Re: [PATCH] hfs: add error checking for hfs_find_init()
> To: "Alexey Khoroshilov" 
> Cc: "Al Viro" , "Artem Bityutskiy" 
> , "Christoph Hellwig" , 
> linux-fsde...@vger.kernel.org, linux-kernel@vger.kernel.org, 
> ldv-proj...@linuxtesting.org, "Hin-Tak Leung" 
> Date: Saturday, 30 March, 2013, 11:35
> Hi Alexey,
> 
> On Mar 30, 2013, at 12:44 AM, Alexey Khoroshilov wrote:
> 
> > hfs_find_init() may fail with ENOMEM, but there are
> places,
> > where the returned value is not checked. The
> consequences can be
> > very unpleasant, e.g. kfree uninitialized pointer and
> > inappropriate mutex unlocking.
> > 
> > The patch adds checks for errors in hfs_find_init().
> > 
> 
> Thank you for your efforts. I have several remarks. Please,
> see below.

Argh, interesting. I wonder if that is related to how I can get the hfsplus 
driver all confused just running 'du' on one large directory in one of my disks 
repeatedly. I'll be interested to trying the hfsplus version out. Perhaps I 
would suggest/add a few dprint/printk's so that there is a sign in dmesg when 
the error condition is triggered.

Hin-Tak
 
> > Found by Linux Driver Verification project
> (linuxtesting.org).
> > 
> > Signed-off-by: Alexey Khoroshilov 
> > ---
> > fs/hfs/catalog.c |   12 +---
> > fs/hfs/dir.c     |    8
> ++--
> > fs/hfs/extent.c  |   48
> +---
> > fs/hfs/hfs_fs.h  |    2 +-
> > fs/hfs/inode.c   |   11
> +--
> > fs/hfs/super.c   |    4 +++-
> > 6 files changed, 61 insertions(+), 24 deletions(-)
> > 
> > diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
> > index 424b033..9569b39 100644
> > --- a/fs/hfs/catalog.c
> > +++ b/fs/hfs/catalog.c
> > @@ -92,7 +92,9 @@ int hfs_cat_create(u32 cnid, struct
> inode *dir, struct qstr *str, struct inode *
> >         return -ENOSPC;
> > 
> >     sb = dir->i_sb;
> > -   
> hfs_find_init(HFS_SB(sb)->cat_tree, );
> > +    err =
> hfs_find_init(HFS_SB(sb)->cat_tree, );
> > +    if (err)
> > +        return err;
> > 
> >     hfs_cat_build_key(sb, fd.search_key,
> cnid, NULL);
> >     entry_size =
> hfs_cat_build_thread(sb, ,
> S_ISDIR(inode->i_mode) ?
> > @@ -214,7 +216,9 @@ int hfs_cat_delete(u32 cnid, struct
> inode *dir, struct qstr *str)
> > 
> >     dprint(DBG_CAT_MOD, "delete_cat:
> %s,%u\n", str ? str->name : NULL, cnid);
> >     sb = dir->i_sb;
> > -   
> hfs_find_init(HFS_SB(sb)->cat_tree, );
> > +    res =
> hfs_find_init(HFS_SB(sb)->cat_tree, );
> > +    if (res)
> > +        return res;
> > 
> >     hfs_cat_build_key(sb, fd.search_key,
> dir->i_ino, str);
> >     res = hfs_brec_find();
> > @@ -281,7 +285,9 @@ int hfs_cat_move(u32 cnid, struct
> inode *src_dir, struct qstr *src_name,
> >     dprint(DBG_CAT_MOD, "rename_cat: %u
> - %lu,%s - %lu,%s\n", cnid, src_dir->i_ino,
> src_name->name,
> >        
> dst_dir->i_ino, dst_name->name);
> >     sb = src_dir->i_sb;
> > -   
> hfs_find_init(HFS_SB(sb)->cat_tree, _fd);
> > +    err =
> hfs_find_init(HFS_SB(sb)->cat_tree, _fd);
> > +    if (err)
> > +        return err;
> >     dst_fd = src_fd;
> > 
> >     /* find the old dir entry and read
> the data */
> > diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
> > index 5f7f1ab..e1c8048 100644
> > --- a/fs/hfs/dir.c
> > +++ b/fs/hfs/dir.c
> > @@ -25,7 +25,9 @@ static struct dentry
> *hfs_lookup(struct inode *dir, struct dentry *dentry,
> >     struct inode *inode = NULL;
> >     int res;
> > 
> > -   
> hfs_find_init(HFS_SB(dir->i_sb)->cat_tree, );
> > +    res =
> hfs_find_init(HFS_SB(dir->i_sb)->cat_tree, );
> > +    if (res)
> > +        return
> ERR_PTR(res);
> >     hfs_cat_build_key(dir->i_sb,
> fd.search_key, dir->i_ino, >d_name);
> >     res = hfs_brec_read(,
> , sizeof(rec));
> >     if (res) {
> > @@ -63,7 +65,9 @@ static int hfs_readdir(struct file
> *filp, void *dirent, filldir_t filldir)
> >     if (filp->f_pos >=
> inode->i_size)
> >         return 0;
> > 
> > -   
> hfs_find_init(HFS_SB(sb)->cat_tree, );
> > +    err =
> hfs_find_init(HFS_SB(sb)->cat_tree, );
> > +    if (err)
> > +        return err;
> >     hfs_cat_build_key(sb, fd.search_key,
> inode->i_ino,

Re: [PATCH] hfs: add error checking for hfs_find_init()

[Hin-Tak Leung]

--- On Sat, 30/3/13, Vyacheslav Dubeyko sl...@dubeyko.com wrote:

 From: Vyacheslav Dubeyko sl...@dubeyko.com
 Subject: Re: [PATCH] hfs: add error checking for hfs_find_init()
 To: Alexey Khoroshilov khoroshi...@ispras.ru
 Cc: Al Viro v...@zeniv.linux.org.uk, Artem Bityutskiy 
 artem.bityuts...@linux.intel.com, Christoph Hellwig h...@lst.de, 
 linux-fsde...@vger.kernel.org, linux-kernel@vger.kernel.org, 
 ldv-proj...@linuxtesting.org, Hin-Tak Leung ht...@users.sourceforge.net
 Date: Saturday, 30 March, 2013, 11:35
 Hi Alexey,
 
 On Mar 30, 2013, at 12:44 AM, Alexey Khoroshilov wrote:
 
  hfs_find_init() may fail with ENOMEM, but there are
 places,
  where the returned value is not checked. The
 consequences can be
  very unpleasant, e.g. kfree uninitialized pointer and
  inappropriate mutex unlocking.
  
  The patch adds checks for errors in hfs_find_init().
  
 
 Thank you for your efforts. I have several remarks. Please,
 see below.

Argh, interesting. I wonder if that is related to how I can get the hfsplus 
driver all confused just running 'du' on one large directory in one of my disks 
repeatedly. I'll be interested to trying the hfsplus version out. Perhaps I 
would suggest/add a few dprint/printk's so that there is a sign in dmesg when 
the error condition is triggered.

Hin-Tak
 
  Found by Linux Driver Verification project
 (linuxtesting.org).
  
  Signed-off-by: Alexey Khoroshilov khoroshi...@ispras.ru
  ---
  fs/hfs/catalog.c |   12 +---
  fs/hfs/dir.c     |    8
 ++--
  fs/hfs/extent.c  |   48
 +---
  fs/hfs/hfs_fs.h  |    2 +-
  fs/hfs/inode.c   |   11
 +--
  fs/hfs/super.c   |    4 +++-
  6 files changed, 61 insertions(+), 24 deletions(-)
  
  diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
  index 424b033..9569b39 100644
  --- a/fs/hfs/catalog.c
  +++ b/fs/hfs/catalog.c
  @@ -92,7 +92,9 @@ int hfs_cat_create(u32 cnid, struct
 inode *dir, struct qstr *str, struct inode *
          return -ENOSPC;
  
      sb = dir-i_sb;
  -   
 hfs_find_init(HFS_SB(sb)-cat_tree, fd);
  +    err =
 hfs_find_init(HFS_SB(sb)-cat_tree, fd);
  +    if (err)
  +        return err;
  
      hfs_cat_build_key(sb, fd.search_key,
 cnid, NULL);
      entry_size =
 hfs_cat_build_thread(sb, entry,
 S_ISDIR(inode-i_mode) ?
  @@ -214,7 +216,9 @@ int hfs_cat_delete(u32 cnid, struct
 inode *dir, struct qstr *str)
  
      dprint(DBG_CAT_MOD, delete_cat:
 %s,%u\n, str ? str-name : NULL, cnid);
      sb = dir-i_sb;
  -   
 hfs_find_init(HFS_SB(sb)-cat_tree, fd);
  +    res =
 hfs_find_init(HFS_SB(sb)-cat_tree, fd);
  +    if (res)
  +        return res;
  
      hfs_cat_build_key(sb, fd.search_key,
 dir-i_ino, str);
      res = hfs_brec_find(fd);
  @@ -281,7 +285,9 @@ int hfs_cat_move(u32 cnid, struct
 inode *src_dir, struct qstr *src_name,
      dprint(DBG_CAT_MOD, rename_cat: %u
 - %lu,%s - %lu,%s\n, cnid, src_dir-i_ino,
 src_name-name,
         
 dst_dir-i_ino, dst_name-name);
      sb = src_dir-i_sb;
  -   
 hfs_find_init(HFS_SB(sb)-cat_tree, src_fd);
  +    err =
 hfs_find_init(HFS_SB(sb)-cat_tree, src_fd);
  +    if (err)
  +        return err;
      dst_fd = src_fd;
  
      /* find the old dir entry and read
 the data */
  diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
  index 5f7f1ab..e1c8048 100644
  --- a/fs/hfs/dir.c
  +++ b/fs/hfs/dir.c
  @@ -25,7 +25,9 @@ static struct dentry
 *hfs_lookup(struct inode *dir, struct dentry *dentry,
      struct inode *inode = NULL;
      int res;
  
  -   
 hfs_find_init(HFS_SB(dir-i_sb)-cat_tree, fd);
  +    res =
 hfs_find_init(HFS_SB(dir-i_sb)-cat_tree, fd);
  +    if (res)
  +        return
 ERR_PTR(res);
      hfs_cat_build_key(dir-i_sb,
 fd.search_key, dir-i_ino, dentry-d_name);
      res = hfs_brec_read(fd,
 rec, sizeof(rec));
      if (res) {
  @@ -63,7 +65,9 @@ static int hfs_readdir(struct file
 *filp, void *dirent, filldir_t filldir)
      if (filp-f_pos =
 inode-i_size)
          return 0;
  
  -   
 hfs_find_init(HFS_SB(sb)-cat_tree, fd);
  +    err =
 hfs_find_init(HFS_SB(sb)-cat_tree, fd);
  +    if (err)
  +        return err;
      hfs_cat_build_key(sb, fd.search_key,
 inode-i_ino, NULL);
      err = hfs_brec_find(fd);
      if (err)
  diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
  index a67955a..813447b 100644
  --- a/fs/hfs/extent.c
  +++ b/fs/hfs/extent.c
  @@ -107,7 +107,7 @@ static u16 hfs_ext_lastblock(struct
 hfs_extent *ext)
      return be16_to_cpu(ext-block) +
 be16_to_cpu(ext-count);
  }
  
  -static void __hfs_ext_write_extent(struct inode
 *inode, struct hfs_find_data *fd)
  +static int __hfs_ext_write_extent(struct inode *inode,
 struct hfs_find_data *fd)
  {
      int res;
  
  @@ -116,26 +116,31 @@ static void
 __hfs_ext_write_extent(struct inode *inode, struct
 hfs_find_data *fd
      res = hfs_brec_find(fd);
      if (HFS_I(inode)-flags 
 HFS_FLG_EXT_NEW) {
          if (res !=
 -ENOENT)
  -       
     return;
  +       
     return res;
         
 hfs_brec_insert(fd, HFS_I(inode

Re: [v3 1/1] driver-core: Shut up dev_dbg_reatelimited() without DEBUG

[Hin-Tak Leung]

--- On Fri, 24/8/12, Hiroshi Doyu  wrote:

> From: Hiroshi Doyu 
> Subject: [v3 1/1] driver-core: Shut up dev_dbg_reatelimited() without DEBUG
> To: "gre...@linuxfoundation.org" 
> Cc: "linux-kernel@vger.kernel.org" , 
> "ht...@users.sourceforge.net" , 
> "linux-me...@vger.kernel.org" , 
> "j...@perches.com" , "linux-te...@vger.kernel.org" 
> , "cr...@iki.fi" 
> Date: Friday, 24 August, 2012, 5:35
> dev_dbg_reatelimited() without DEBUG
> printed "217078 callbacks
> suppressed". This shouldn't print anything without DEBUG.
> 
> With CONFIG_DYNAMIC_DEBUG, the print should be configured as
> expected.
> 
> Signed-off-by: Hiroshi Doyu 
> Reported-by: Hin-Tak Leung 
> Tested-by: Antti Palosaari 
> Acked-by: Hin-Tak Leung 

Tested-by: Hin-Tak Leung 

Went ahead and patched my 2.5.x distro kernel-devel package header, and it 
works as expected. Apologies about the red-herring with media_build (for those 
who are not familar with it, = "back-port" wrapper package for building new DVB 
modules against older kernels). 

The distro kernel-devel headers is per installed distro kernel so will be 
replaced in a week or two... no permanent demage done :-).   

> ---
>  include/linux/device.h |   62
> +--
>  1 files changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/device.h
> b/include/linux/device.h
> index 9648331..bb6ffcb 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -932,6 +932,32 @@ int _dev_info(const struct device *dev,
> const char *fmt, ...)
>  
>  #endif
>  
> +/*
> + * Stupid hackaround for existing uses of non-printk uses
> dev_info
> + *
> + * Note that the definition of dev_info below is actually
> _dev_info
> + * and a macro is used to avoid redefining dev_info
> + */
> +
> +#define dev_info(dev, fmt, arg...) _dev_info(dev, fmt,
> ##arg)
> +
> +#if defined(CONFIG_DYNAMIC_DEBUG)
> +#define dev_dbg(dev, format, ...)   
>          \
> +do {       
>            
>          \
> +    dynamic_dev_dbg(dev, format,
> ##__VA_ARGS__); \
> +} while (0)
> +#elif defined(DEBUG)
> +#define dev_dbg(dev, format, arg...)   
>     \
> +    dev_printk(KERN_DEBUG, dev, format,
> ##arg)
> +#else
> +#define dev_dbg(dev, format, arg...)   
>             \
> +({           
>            
>         \
> +    if (0)   
>            
>             \
> +       
> dev_printk(KERN_DEBUG, dev, format,
> ##arg);    \
> +    0;       
>            
>         \
> +})
> +#endif
> +
>  #define dev_level_ratelimited(dev_level, dev, fmt,
> ...)           
> \
>  do {       
>            
>            
>     \
>      static
> DEFINE_RATELIMIT_STATE(_rs,   
>             \
> @@ -955,33 +981,21 @@ do {   
>            
>            
>         \
>      dev_level_ratelimited(dev_notice, dev,
> fmt, ##__VA_ARGS__)
>  #define dev_info_ratelimited(dev, fmt,
> ...)           
>     \
>      dev_level_ratelimited(dev_info, dev,
> fmt, ##__VA_ARGS__)
> +#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
>  #define dev_dbg_ratelimited(dev, fmt,
> ...)           
>     \
> -    dev_level_ratelimited(dev_dbg, dev, fmt,
> ##__VA_ARGS__)
> -
> -/*
> - * Stupid hackaround for existing uses of non-printk uses
> dev_info
> - *
> - * Note that the definition of dev_info below is actually
> _dev_info
> - * and a macro is used to avoid redefining dev_info
> - */
> -
> -#define dev_info(dev, fmt, arg...) _dev_info(dev, fmt,
> ##arg)
> -
> -#if defined(CONFIG_DYNAMIC_DEBUG)
> -#define dev_dbg(dev, format, ...)   
>          \
> -do {       
>            
>          \
> -    dynamic_dev_dbg(dev, format,
> ##__VA_ARGS__); \
> +do {       
>            
>            
>     \
> +    static
> DEFINE_RATELIMIT_STATE(_rs,   
>             \
> +           
>          
> DEFAULT_RATELIMIT_INTERVAL,    \
> +           
>          
> DEFAULT_RATELIMIT_BURST);   
>     \
> +   
> DEFINE_DYNAMIC_DEBUG_METADATA(descriptor,
> fmt);       
>     \
> +    if (unlikely(descriptor.flags &
> _DPRINTK_FLAGS_PRINT) &&    \
> +       
> __ratelimit(&_rs))       
>            
>     \
> +       
> __dynamic_pr_debug(,
> pr_fmt(fmt),        \
> +           
>    
>    ##__VA_ARGS__);   
>         \
>  } while (0)
> -#elif defined(DEBUG)
> -#define dev_dbg(dev, format, arg...)   
>     \
> -    dev_printk(KERN_DEBUG, dev, format,
> ##arg)
>  #else
> -#define dev_dbg(dev, format, arg...)   
>             

Re: [v3 1/1] driver-core: Shut up dev_dbg_reatelimited() without DEBUG

[Hin-Tak Leung]

--- On Fri, 24/8/12, Hiroshi Doyu hd...@nvidia.com wrote:

 From: Hiroshi Doyu hd...@nvidia.com
 Subject: [v3 1/1] driver-core: Shut up dev_dbg_reatelimited() without DEBUG
 To: gre...@linuxfoundation.org gre...@linuxfoundation.org
 Cc: linux-kernel@vger.kernel.org linux-kernel@vger.kernel.org, 
 ht...@users.sourceforge.net ht...@users.sourceforge.net, 
 linux-me...@vger.kernel.org linux-me...@vger.kernel.org, 
 j...@perches.com j...@perches.com, linux-te...@vger.kernel.org 
 linux-te...@vger.kernel.org, cr...@iki.fi cr...@iki.fi
 Date: Friday, 24 August, 2012, 5:35
 dev_dbg_reatelimited() without DEBUG
 printed 217078 callbacks
 suppressed. This shouldn't print anything without DEBUG.
 
 With CONFIG_DYNAMIC_DEBUG, the print should be configured as
 expected.
 
 Signed-off-by: Hiroshi Doyu hd...@nvidia.com
 Reported-by: Hin-Tak Leung ht...@users.sourceforge.net
 Tested-by: Antti Palosaari cr...@iki.fi
 Acked-by: Hin-Tak Leung ht...@users.sourceforge.net

Tested-by: Hin-Tak Leung ht...@users.sourceforge.net

Went ahead and patched my 2.5.x distro kernel-devel package header, and it 
works as expected. Apologies about the red-herring with media_build (for those 
who are not familar with it, = back-port wrapper package for building new DVB 
modules against older kernels). 

The distro kernel-devel headers is per installed distro kernel so will be 
replaced in a week or two... no permanent demage done :-).   

 ---
  include/linux/device.h |   62
 +--
  1 files changed, 38 insertions(+), 24 deletions(-)
 
 diff --git a/include/linux/device.h
 b/include/linux/device.h
 index 9648331..bb6ffcb 100644
 --- a/include/linux/device.h
 +++ b/include/linux/device.h
 @@ -932,6 +932,32 @@ int _dev_info(const struct device *dev,
 const char *fmt, ...)
  
  #endif
  
 +/*
 + * Stupid hackaround for existing uses of non-printk uses
 dev_info
 + *
 + * Note that the definition of dev_info below is actually
 _dev_info
 + * and a macro is used to avoid redefining dev_info
 + */
 +
 +#define dev_info(dev, fmt, arg...) _dev_info(dev, fmt,
 ##arg)
 +
 +#if defined(CONFIG_DYNAMIC_DEBUG)
 +#define dev_dbg(dev, format, ...)   
          \
 +do {       
            
          \
 +    dynamic_dev_dbg(dev, format,
 ##__VA_ARGS__); \
 +} while (0)
 +#elif defined(DEBUG)
 +#define dev_dbg(dev, format, arg...)   
     \
 +    dev_printk(KERN_DEBUG, dev, format,
 ##arg)
 +#else
 +#define dev_dbg(dev, format, arg...)   
             \
 +({           
            
         \
 +    if (0)   
            
             \
 +       
 dev_printk(KERN_DEBUG, dev, format,
 ##arg);    \
 +    0;       
            
         \
 +})
 +#endif
 +
  #define dev_level_ratelimited(dev_level, dev, fmt,
 ...)           
 \
  do {       
            
            
     \
      static
 DEFINE_RATELIMIT_STATE(_rs,   
             \
 @@ -955,33 +981,21 @@ do {   
            
            
         \
      dev_level_ratelimited(dev_notice, dev,
 fmt, ##__VA_ARGS__)
  #define dev_info_ratelimited(dev, fmt,
 ...)           
     \
      dev_level_ratelimited(dev_info, dev,
 fmt, ##__VA_ARGS__)
 +#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
  #define dev_dbg_ratelimited(dev, fmt,
 ...)           
     \
 -    dev_level_ratelimited(dev_dbg, dev, fmt,
 ##__VA_ARGS__)
 -
 -/*
 - * Stupid hackaround for existing uses of non-printk uses
 dev_info
 - *
 - * Note that the definition of dev_info below is actually
 _dev_info
 - * and a macro is used to avoid redefining dev_info
 - */
 -
 -#define dev_info(dev, fmt, arg...) _dev_info(dev, fmt,
 ##arg)
 -
 -#if defined(CONFIG_DYNAMIC_DEBUG)
 -#define dev_dbg(dev, format, ...)   
          \
 -do {       
            
          \
 -    dynamic_dev_dbg(dev, format,
 ##__VA_ARGS__); \
 +do {       
            
            
     \
 +    static
 DEFINE_RATELIMIT_STATE(_rs,   
             \
 +           
          
 DEFAULT_RATELIMIT_INTERVAL,    \
 +           
          
 DEFAULT_RATELIMIT_BURST);   
     \
 +   
 DEFINE_DYNAMIC_DEBUG_METADATA(descriptor,
 fmt);       
     \
 +    if (unlikely(descriptor.flags 
 _DPRINTK_FLAGS_PRINT)     \
 +       
 __ratelimit(_rs))       
            
     \
 +       
 __dynamic_pr_debug(descriptor,
 pr_fmt(fmt),        \
 +           
    
    ##__VA_ARGS__);   
         \
  } while (0)
 -#elif defined(DEBUG)
 -#define dev_dbg(dev, format, arg...)   
     \
 -    dev_printk(KERN_DEBUG, dev, format,
 ##arg)
  #else
 -#define dev_dbg(dev, format, arg...)   
             \
 -({           
            
         \
 -    if (0)   
            
             \
 -       
 dev_printk(KERN_DEBUG, dev, format,
 ##arg);    \
 -    0;       
            
         \
 -})
 +#define dev_dbg_ratelimited(dev, fmt,
 ...)           
 \
 +    no_printk(KERN_DEBUG pr_fmt(fmt),
 ##__VA_ARGS__)
  #endif
  
  #ifdef VERBOSE_DEBUG
 -- 
 1.7.5.4
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord

Re: [PATCH 1/1] driver-core: Shut up dev_dbg_reatelimited() without DEBUG

[Hin-Tak Leung]

--- On Wed, 22/8/12, Antti Palosaari  wrote:

> On 08/22/2012 04:57 PM, Hin-Tak Leung
> wrote:
> > Antti Palosaari wrote:
> >> Hello Hiroshi,
> >>
> >> On 08/21/2012 10:02 AM, Hiroshi Doyu wrote:
> >>> Antti Palosaari 
> wrote @ Mon, 20 Aug 2012 23:29:34 +0200:
> >>>
> >>>> On 08/20/2012 02:14 PM, Hiroshi Doyu
> wrote:
> >>>>> Hi Antti,
> >>>>>
> >>>>> Antti Palosaari 
> wrote @ Sat, 18 Aug 2012 02:11:56
> >>>>> +0200:
> >>>>>
> >>>>>> On 08/17/2012 09:04 AM, Hiroshi
> Doyu wrote:
> >>>>>>> dev_dbg_reatelimited() without
> DEBUG printed "217078 callbacks
> >>>>>>> suppressed". This shouldn't
> print anything without DEBUG.
> >>>>>>>
> >>>>>>> Signed-off-by: Hiroshi Doyu
> 
> >>>>>>> Reported-by: Antti Palosaari
> 
> >>>>>>> ---
> >>>>>>> 
>    include/linux/device.h |    6
> +-
> >>>>>>>     1 files
> changed, 5 insertions(+), 1 deletions(-)
> >>>>>>>
> >>>>>>> diff --git
> a/include/linux/device.h b/include/linux/device.h
> >>>>>>> index eb945e1..d4dc26e 100644
> >>>>>>> --- a/include/linux/device.h
> >>>>>>> +++ b/include/linux/device.h
> >>>>>>> @@ -962,9 +962,13 @@ do { 
>                
>                
>   \
> >>>>>>>     
>    dev_level_ratelimited(dev_notice, dev,
> fmt, ##__VA_ARGS__)
> >>>>>>>     #define
> dev_info_ratelimited(dev, fmt, ...)     
>           \
> >>>>>>>     
>    dev_level_ratelimited(dev_info, dev, fmt,
> ##__VA_ARGS__)
> >>>>>>> +#if defined(DEBUG)
> >>>>>>>     #define
> dev_dbg_ratelimited(dev, fmt, ...)     
>           \
> >>>>>>>     
>    dev_level_ratelimited(dev_dbg, dev, fmt,
> ##__VA_ARGS__)
> >>>>>>> -
> >>>>>>> +#else
> >>>>>>> +#define
> dev_dbg_ratelimited(dev, fmt, ...)     
>       \
> >>>>>>> +   
> no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> >>>>>>> +#endif
> >>>>>>>     /*
> >>>>>>>      * Stupid
> hackaround for existing uses of non-printk uses
> >>>>>>> dev_info
> >>>>>>>      *
> >>>>>>>
> >>>>>>
> >>>>>> NACK. I don't think that's correct
> behavior. After that patch it
> >>>>>> kills
> >>>>>> all output of
> dev_dbg_ratelimited(). If I use dynamic debugs and
> >>>>>> order
> >>>>>> debugs, I expect to see debugs as
> earlier.
> >>>>>
> >>>>> You are right. I attached the update
> patch, just moving *_ratelimited
> >>>>> functions after dev_dbg() definitions.
> >>>>>
> >>>>> With DEBUG defined/undefined in your
> "test.ko", it works fine. With
> >>>>> CONFIG_DYNAMIC_DEBUG, it works with
> "+p", but with "-p", still
> >>>>> "..callbacks suppressed" is printed.
> >>>>
> >>>> I am using dynamic debugs and behavior is
> now just same as it was when
> >>>> reported that bug. OK, likely for static
> debug it is now correct.
> >>>
> >>> The following patch can also refrain
> "..callbacks suppressed" with
> >>> "-p". I think that it's ok for all cases.
> >>>
> >>>> From
> b4c6aa9160f03b61ed17975c73db36c983a48927 Mon Sep 17 00:00:00
> 2001
> >>> From: Hiroshi Doyu 
> >>> Date: Mon, 20 Aug 2012 13:49:19 +0300
> >>> Subject: [v3 1/1] driver-core: Shut up
> dev_dbg_reatelimited() without
> >>> DEBUG
> >>>
> >>> dev_dbg_reatelimited() without DEBUG printed
> "217078 callbacks
> >>> suppressed". This shouldn't print anything
> without DEBUG.
> >>>
> >>> With CONFIG_DYNAMIC_DEBUG, the print should be
> configured as expected.
> >>>
> >>> Signed-off-by: Hiroshi Doyu 
> >>> Reported-by: Antti Palosaari 
> >>> ---
> >>>   includ

Re: [PATCH 1/1] driver-core: Shut up dev_dbg_reatelimited() without DEBUG

[Hin-Tak Leung]

--- On Wed, 22/8/12, Antti Palosaari cr...@iki.fi wrote:

 On 08/22/2012 04:57 PM, Hin-Tak Leung
 wrote:
  Antti Palosaari wrote:
  Hello Hiroshi,
 
  On 08/21/2012 10:02 AM, Hiroshi Doyu wrote:
  Antti Palosaari cr...@iki.fi
 wrote @ Mon, 20 Aug 2012 23:29:34 +0200:
 
  On 08/20/2012 02:14 PM, Hiroshi Doyu
 wrote:
  Hi Antti,
 
  Antti Palosaari cr...@iki.fi
 wrote @ Sat, 18 Aug 2012 02:11:56
  +0200:
 
  On 08/17/2012 09:04 AM, Hiroshi
 Doyu wrote:
  dev_dbg_reatelimited() without
 DEBUG printed 217078 callbacks
  suppressed. This shouldn't
 print anything without DEBUG.
 
  Signed-off-by: Hiroshi Doyu
 hd...@nvidia.com
  Reported-by: Antti Palosaari
 cr...@iki.fi
  ---
  
    include/linux/device.h |    6
 +-
      1 files
 changed, 5 insertions(+), 1 deletions(-)
 
  diff --git
 a/include/linux/device.h b/include/linux/device.h
  index eb945e1..d4dc26e 100644
  --- a/include/linux/device.h
  +++ b/include/linux/device.h
  @@ -962,9 +962,13 @@ do { 
                
                
   \
      
    dev_level_ratelimited(dev_notice, dev,
 fmt, ##__VA_ARGS__)
      #define
 dev_info_ratelimited(dev, fmt, ...)     
           \
      
    dev_level_ratelimited(dev_info, dev, fmt,
 ##__VA_ARGS__)
  +#if defined(DEBUG)
      #define
 dev_dbg_ratelimited(dev, fmt, ...)     
           \
      
    dev_level_ratelimited(dev_dbg, dev, fmt,
 ##__VA_ARGS__)
  -
  +#else
  +#define
 dev_dbg_ratelimited(dev, fmt, ...)     
       \
  +   
 no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
  +#endif
      /*
       * Stupid
 hackaround for existing uses of non-printk uses
  dev_info
       *
 
 
  NACK. I don't think that's correct
 behavior. After that patch it
  kills
  all output of
 dev_dbg_ratelimited(). If I use dynamic debugs and
  order
  debugs, I expect to see debugs as
 earlier.
 
  You are right. I attached the update
 patch, just moving *_ratelimited
  functions after dev_dbg() definitions.
 
  With DEBUG defined/undefined in your
 test.ko, it works fine. With
  CONFIG_DYNAMIC_DEBUG, it works with
 +p, but with -p, still
  ..callbacks suppressed is printed.
 
  I am using dynamic debugs and behavior is
 now just same as it was when
  reported that bug. OK, likely for static
 debug it is now correct.
 
  The following patch can also refrain
 ..callbacks suppressed with
  -p. I think that it's ok for all cases.
 
  From
 b4c6aa9160f03b61ed17975c73db36c983a48927 Mon Sep 17 00:00:00
 2001
  From: Hiroshi Doyu hd...@nvidia.com
  Date: Mon, 20 Aug 2012 13:49:19 +0300
  Subject: [v3 1/1] driver-core: Shut up
 dev_dbg_reatelimited() without
  DEBUG
 
  dev_dbg_reatelimited() without DEBUG printed
 217078 callbacks
  suppressed. This shouldn't print anything
 without DEBUG.
 
  With CONFIG_DYNAMIC_DEBUG, the print should be
 configured as expected.
 
  Signed-off-by: Hiroshi Doyu hd...@nvidia.com
  Reported-by: Antti Palosaari cr...@iki.fi
  ---
    include/linux/device.h
 |   62
 
 +--
    1 files changed, 38
 insertions(+), 24 deletions(-)
 
  diff --git a/include/linux/device.h
 b/include/linux/device.h
  index 9648331..bb6ffcb 100644
  --- a/include/linux/device.h
  +++ b/include/linux/device.h
  @@ -932,6 +932,32 @@ int _dev_info(const struct
 device *dev, const
  char *fmt,
  ...)
 
    #endif
 
  +/*
  + * Stupid hackaround for existing uses of
 non-printk uses dev_info
  + *
  + * Note that the definition of dev_info below
 is actually _dev_info
  + * and a macro is used to avoid redefining
 dev_info
  + */
  +
  +#define dev_info(dev, fmt, arg...)
 _dev_info(dev, fmt, ##arg)
  +
  +#if defined(CONFIG_DYNAMIC_DEBUG)
  +#define dev_dbg(dev, format, ...)   
          \
  +do {           
              
    \
  +    dynamic_dev_dbg(dev, format,
 ##__VA_ARGS__); \
  +} while (0)
  +#elif defined(DEBUG)
  +#define dev_dbg(dev, format, arg...) 
       \
  +    dev_printk(KERN_DEBUG, dev,
 format, ##arg)
  +#else
  +#define dev_dbg(dev, format, arg...) 
               \
  +({           
                
     \
  +    if (0)     
                
       \
  +       
 dev_printk(KERN_DEBUG, dev, format, ##arg);    \
  +    0;       
                
     \
  +})
  +#endif
  +
    #define
 dev_level_ratelimited(dev_level, dev, fmt, ...)   
         \
    do {     
                
               \
        static
 DEFINE_RATELIMIT_STATE(_rs,       
         \
  @@ -955,33 +981,21 @@ do {     
                
               \
    
    dev_level_ratelimited(dev_notice, dev,
 fmt, ##__VA_ARGS__)
    #define
 dev_info_ratelimited(dev, fmt, ...)     
           \
    
    dev_level_ratelimited(dev_info, dev, fmt,
 ##__VA_ARGS__)
  +#if defined(CONFIG_DYNAMIC_DEBUG) ||
 defined(DEBUG)
    #define
 dev_dbg_ratelimited(dev, fmt, ...)     
           \
  -    dev_level_ratelimited(dev_dbg,
 dev, fmt, ##__VA_ARGS__)
  -
  -/*
  - * Stupid hackaround for existing uses of
 non-printk uses dev_info
  - *
  - * Note that the definition of dev_info below
 is actually _dev_info

Re: [PATCH 1/1] driver-core: Shut up dev_dbg_reatelimited() without DEBUG

[Hin-Tak Leung]

 DEFAULT_RATELIMIT_INTERVAL,\
+  DEFAULT_RATELIMIT_BURST);\
+DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);\
+if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) &&\
+__ratelimit(&_rs))\
+__dynamic_pr_debug(, pr_fmt(fmt),\
+   ##__VA_ARGS__);\
  } while (0)
-#elif defined(DEBUG)
-#define dev_dbg(dev, format, arg...)\
-dev_printk(KERN_DEBUG, dev, format, ##arg)
  #else
-#define dev_dbg(dev, format, arg...)\
-({\
-if (0)\
-dev_printk(KERN_DEBUG, dev, format, ##arg);\
-0;\
-})
+#define dev_dbg_ratelimited(dev, fmt, ...)\
+no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
  #endif

  #ifdef VERBOSE_DEBUG


That seems to work correctly now. I tested it using dynamic debugs. It was
Hin-Tak who originally reported that bug for me after I added few ratelimited
debugs for DVB stack. Thank you!

Reported-by: Hin-Tak Leung 
Tested-by: Antti Palosaari 


regards
Antti



This is with mediatree/for_v3.7-8 , playing DVB-T video with mplayer.

echo 'file ...media_build/v4l/usb_urb.c +p' > 
/sys/kernel/debug/dynamic_debug/control


With +p

[137749.698202] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137749.699449] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137749.700825] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0

[137754.690862] usb_urb_complete: 3570 callbacks suppressed
[137754.690888] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.692489] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.693745] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.694882] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.696240] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.697483] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.699002] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.700884] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.701613] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.702986] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0

[137759.695906] usb_urb_complete: 3595 callbacks suppressed
[137759.695934] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137759.697788] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137759.698772] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0


with -p

[137814.730303] usb_urb_complete: 3555 callbacks suppressed
[137819.740698] usb_urb_complete: 3519 callbacks suppressed
[137824.744857] usb_urb_complete: 3443 callbacks suppressed
[137829.746023] usb_urb_complete: 3345 callbacks suppressed
[137834.749931] usb_urb_complete: 3558 callbacks suppressed
[137839.753102] usb_urb_complete: 3465 callbacks suppressed
[137844.755521] usb_urb_complete: 3438 callbacks suppressed

--
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 1/1] driver-core: Shut up dev_dbg_reatelimited() without DEBUG

[Hin-Tak Leung]

,\
+  DEFAULT_RATELIMIT_INTERVAL,\
+  DEFAULT_RATELIMIT_BURST);\
+DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);\
+if (unlikely(descriptor.flags  _DPRINTK_FLAGS_PRINT) \
+__ratelimit(_rs))\
+__dynamic_pr_debug(descriptor, pr_fmt(fmt),\
+   ##__VA_ARGS__);\
  } while (0)
-#elif defined(DEBUG)
-#define dev_dbg(dev, format, arg...)\
-dev_printk(KERN_DEBUG, dev, format, ##arg)
  #else
-#define dev_dbg(dev, format, arg...)\
-({\
-if (0)\
-dev_printk(KERN_DEBUG, dev, format, ##arg);\
-0;\
-})
+#define dev_dbg_ratelimited(dev, fmt, ...)\
+no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
  #endif

  #ifdef VERBOSE_DEBUG


That seems to work correctly now. I tested it using dynamic debugs. It was
Hin-Tak who originally reported that bug for me after I added few ratelimited
debugs for DVB stack. Thank you!

Reported-by: Hin-Tak Leung ht...@users.sourceforge.net
Tested-by: Antti Palosaari cr...@iki.fi


regards
Antti



This is with mediatree/for_v3.7-8 , playing DVB-T video with mplayer.

echo 'file ...media_build/v4l/usb_urb.c +p'  
/sys/kernel/debug/dynamic_debug/control


With +p

[137749.698202] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137749.699449] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137749.700825] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0

[137754.690862] usb_urb_complete: 3570 callbacks suppressed
[137754.690888] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.692489] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.693745] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.694882] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.696240] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.697483] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.699002] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.700884] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.701613] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137754.702986] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0

[137759.695906] usb_urb_complete: 3595 callbacks suppressed
[137759.695934] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137759.697788] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0
[137759.698772] usb 1-3: usb_urb_complete: bulk urb completed status=0 
length=4096/4096 pack_num=0 errors=0


with -p

[137814.730303] usb_urb_complete: 3555 callbacks suppressed
[137819.740698] usb_urb_complete: 3519 callbacks suppressed
[137824.744857] usb_urb_complete: 3443 callbacks suppressed
[137829.746023] usb_urb_complete: 3345 callbacks suppressed
[137834.749931] usb_urb_complete: 3558 callbacks suppressed
[137839.753102] usb_urb_complete: 3465 callbacks suppressed
[137844.755521] usb_urb_complete: 3438 callbacks suppressed

--
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 net-next 4/8] wireless: Use eth_random_addr

[Hin-Tak Leung]

--- On Fri, 13/7/12, Joe Perches  wrote:

> From: Joe Perches 
> Subject: [PATCH net-next 4/8] wireless: Use eth_random_addr
> To: "David Miller" , "John W. Linville" 
> , "Christian Lamparter" , 
> "Ivo van Doorn" , "Gertjan van Wingerde" 
> , "Helmut Schaa" , "Herton 
> Ronaldo Krzesinski" , "Hin-Tak Leung" 
> , "Larry Finger" 
> Cc: "Johannes Berg" , 
> linux-wirel...@vger.kernel.org, net...@vger.kernel.org, 
> linux-kernel@vger.kernel.org, us...@rt2x00.serialmonkey.com
> Date: Friday, 13 July, 2012, 6:33
> Convert the existing uses of
> random_ether_addr to
> the new eth_random_addr.
> 
> Signed-off-by: Joe Perches 

Acked-by: Hin-Tak Leung 

Would it make sense to have a "check & set" macro? 

> ---
>  drivers/net/wireless/adm8211.c       
>      |    2 +-
>  drivers/net/wireless/p54/eeprom.c     
>     |    2 +-
>  drivers/net/wireless/rt2x00/rt2400pci.c   
> |    2 +-
>  drivers/net/wireless/rt2x00/rt2500pci.c   
> |    2 +-
>  drivers/net/wireless/rt2x00/rt2500usb.c   
> |    2 +-
>  drivers/net/wireless/rt2x00/rt2800lib.c   
> |    2 +-
>  drivers/net/wireless/rt2x00/rt61pci.c     
> |    2 +-
>  drivers/net/wireless/rt2x00/rt73usb.c     
> |    2 +-
>  drivers/net/wireless/rtl818x/rtl8180/dev.c |    2
> +-
>  drivers/net/wireless/rtl818x/rtl8187/dev.c |    2
> +-
>  10 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/adm8211.c
> b/drivers/net/wireless/adm8211.c
> index 97afcec..689a71c 100644
> --- a/drivers/net/wireless/adm8211.c
> +++ b/drivers/net/wireless/adm8211.c
> @@ -1854,7 +1854,7 @@ static int __devinit
> adm8211_probe(struct pci_dev *pdev,
>      if (!is_valid_ether_addr(perm_addr)) {
>          printk(KERN_WARNING
> "%s (adm8211): Invalid hwaddr in EEPROM!\n",
>             
>    pci_name(pdev));
> -       
> random_ether_addr(perm_addr);
> +       
> eth_random_addr(perm_addr);
>      }
>      SET_IEEE80211_PERM_ADDR(dev,
> perm_addr);
>  
> diff --git a/drivers/net/wireless/p54/eeprom.c
> b/drivers/net/wireless/p54/eeprom.c
> index 636daf2..1403709 100644
> --- a/drivers/net/wireless/p54/eeprom.c
> +++ b/drivers/net/wireless/p54/eeprom.c
> @@ -857,7 +857,7 @@ good_eeprom:
>  
>         
> wiphy_warn(dev->wiphy,
>             
>    "Invalid hwaddr! Using randomly generated
> MAC addr\n");
> -       
> random_ether_addr(perm_addr);
> +       
> eth_random_addr(perm_addr);
>         
> SET_IEEE80211_PERM_ADDR(dev, perm_addr);
>      }
>  
> diff --git a/drivers/net/wireless/rt2x00/rt2400pci.c
> b/drivers/net/wireless/rt2x00/rt2400pci.c
> index 5e6b501..8b9dbd7 100644
> --- a/drivers/net/wireless/rt2x00/rt2400pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2400pci.c
> @@ -1455,7 +1455,7 @@ static int
> rt2400pci_validate_eeprom(struct rt2x00_dev *rt2x00dev)
>   */
>      mac = rt2x00_eeprom_addr(rt2x00dev,
> EEPROM_MAC_ADDR_0);
>      if (!is_valid_ether_addr(mac)) {
> -       
> random_ether_addr(mac);
> +       
> eth_random_addr(mac);
>          EEPROM(rt2x00dev,
> "MAC: %pM\n", mac);
>      }
>  
> diff --git a/drivers/net/wireless/rt2x00/rt2500pci.c
> b/drivers/net/wireless/rt2x00/rt2500pci.c
> index 136b849..d2cf8a4 100644
> --- a/drivers/net/wireless/rt2x00/rt2500pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2500pci.c
> @@ -1585,7 +1585,7 @@ static int
> rt2500pci_validate_eeprom(struct rt2x00_dev *rt2x00dev)
>   */
>      mac = rt2x00_eeprom_addr(rt2x00dev,
> EEPROM_MAC_ADDR_0);
>      if (!is_valid_ether_addr(mac)) {
> -       
> random_ether_addr(mac);
> +       
> eth_random_addr(mac);
>          EEPROM(rt2x00dev,
> "MAC: %pM\n", mac);
>      }
>  
> diff --git a/drivers/net/wireless/rt2x00/rt2500usb.c
> b/drivers/net/wireless/rt2x00/rt2500usb.c
> index 669aecd..3aae36b 100644
> --- a/drivers/net/wireless/rt2x00/rt2500usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2500usb.c
> @@ -1352,7 +1352,7 @@ static int
> rt2500usb_validate_eeprom(struct rt2x00_dev *rt2x00dev)
>   */
>      mac = rt2x00_eeprom_addr(rt2x00dev,
> EEPROM_MAC_ADDR_0);
>      if (!is_valid_ether_addr(mac)) {
> -       
> random_ether_addr(mac);
> +       
> eth_random_addr(mac);
>          EEPROM(rt2x00dev,
> "MAC: %pM\n", mac);
>      }
>  
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c
> b/drivers/net/wireless/rt2x00/rt2800lib.c
> index 068276e..d857d55 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -4340,7 +4340

Re: [PATCH net-next 4/8] wireless: Use eth_random_addr

[Hin-Tak Leung]

--- On Fri, 13/7/12, Joe Perches j...@perches.com wrote:

 From: Joe Perches j...@perches.com
 Subject: [PATCH net-next 4/8] wireless: Use eth_random_addr
 To: David Miller da...@davemloft.net, John W. Linville 
 linvi...@tuxdriver.com, Christian Lamparter chunk...@googlemail.com, 
 Ivo van Doorn ivdo...@gmail.com, Gertjan van Wingerde 
 gwinge...@gmail.com, Helmut Schaa helmut.sc...@googlemail.com, Herton 
 Ronaldo Krzesinski her...@canonical.com, Hin-Tak Leung 
 ht...@users.sourceforge.net, Larry Finger larry.fin...@lwfinger.net
 Cc: Johannes Berg johan...@sipsolutions.net, 
 linux-wirel...@vger.kernel.org, net...@vger.kernel.org, 
 linux-kernel@vger.kernel.org, us...@rt2x00.serialmonkey.com
 Date: Friday, 13 July, 2012, 6:33
 Convert the existing uses of
 random_ether_addr to
 the new eth_random_addr.
 
 Signed-off-by: Joe Perches j...@perches.com

Acked-by: Hin-Tak Leung ht...@users.sourceforge.net

Would it make sense to have a check  set macro? 

 ---
  drivers/net/wireless/adm8211.c       
      |    2 +-
  drivers/net/wireless/p54/eeprom.c     
     |    2 +-
  drivers/net/wireless/rt2x00/rt2400pci.c   
 |    2 +-
  drivers/net/wireless/rt2x00/rt2500pci.c   
 |    2 +-
  drivers/net/wireless/rt2x00/rt2500usb.c   
 |    2 +-
  drivers/net/wireless/rt2x00/rt2800lib.c   
 |    2 +-
  drivers/net/wireless/rt2x00/rt61pci.c     
 |    2 +-
  drivers/net/wireless/rt2x00/rt73usb.c     
 |    2 +-
  drivers/net/wireless/rtl818x/rtl8180/dev.c |    2
 +-
  drivers/net/wireless/rtl818x/rtl8187/dev.c |    2
 +-
  10 files changed, 10 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/net/wireless/adm8211.c
 b/drivers/net/wireless/adm8211.c
 index 97afcec..689a71c 100644
 --- a/drivers/net/wireless/adm8211.c
 +++ b/drivers/net/wireless/adm8211.c
 @@ -1854,7 +1854,7 @@ static int __devinit
 adm8211_probe(struct pci_dev *pdev,
      if (!is_valid_ether_addr(perm_addr)) {
          printk(KERN_WARNING
 %s (adm8211): Invalid hwaddr in EEPROM!\n,
             
    pci_name(pdev));
 -       
 random_ether_addr(perm_addr);
 +       
 eth_random_addr(perm_addr);
      }
      SET_IEEE80211_PERM_ADDR(dev,
 perm_addr);
  
 diff --git a/drivers/net/wireless/p54/eeprom.c
 b/drivers/net/wireless/p54/eeprom.c
 index 636daf2..1403709 100644
 --- a/drivers/net/wireless/p54/eeprom.c
 +++ b/drivers/net/wireless/p54/eeprom.c
 @@ -857,7 +857,7 @@ good_eeprom:
  
         
 wiphy_warn(dev-wiphy,
             
    Invalid hwaddr! Using randomly generated
 MAC addr\n);
 -       
 random_ether_addr(perm_addr);
 +       
 eth_random_addr(perm_addr);
         
 SET_IEEE80211_PERM_ADDR(dev, perm_addr);
      }
  
 diff --git a/drivers/net/wireless/rt2x00/rt2400pci.c
 b/drivers/net/wireless/rt2x00/rt2400pci.c
 index 5e6b501..8b9dbd7 100644
 --- a/drivers/net/wireless/rt2x00/rt2400pci.c
 +++ b/drivers/net/wireless/rt2x00/rt2400pci.c
 @@ -1455,7 +1455,7 @@ static int
 rt2400pci_validate_eeprom(struct rt2x00_dev *rt2x00dev)
   */
      mac = rt2x00_eeprom_addr(rt2x00dev,
 EEPROM_MAC_ADDR_0);
      if (!is_valid_ether_addr(mac)) {
 -       
 random_ether_addr(mac);
 +       
 eth_random_addr(mac);
          EEPROM(rt2x00dev,
 MAC: %pM\n, mac);
      }
  
 diff --git a/drivers/net/wireless/rt2x00/rt2500pci.c
 b/drivers/net/wireless/rt2x00/rt2500pci.c
 index 136b849..d2cf8a4 100644
 --- a/drivers/net/wireless/rt2x00/rt2500pci.c
 +++ b/drivers/net/wireless/rt2x00/rt2500pci.c
 @@ -1585,7 +1585,7 @@ static int
 rt2500pci_validate_eeprom(struct rt2x00_dev *rt2x00dev)
   */
      mac = rt2x00_eeprom_addr(rt2x00dev,
 EEPROM_MAC_ADDR_0);
      if (!is_valid_ether_addr(mac)) {
 -       
 random_ether_addr(mac);
 +       
 eth_random_addr(mac);
          EEPROM(rt2x00dev,
 MAC: %pM\n, mac);
      }
  
 diff --git a/drivers/net/wireless/rt2x00/rt2500usb.c
 b/drivers/net/wireless/rt2x00/rt2500usb.c
 index 669aecd..3aae36b 100644
 --- a/drivers/net/wireless/rt2x00/rt2500usb.c
 +++ b/drivers/net/wireless/rt2x00/rt2500usb.c
 @@ -1352,7 +1352,7 @@ static int
 rt2500usb_validate_eeprom(struct rt2x00_dev *rt2x00dev)
   */
      mac = rt2x00_eeprom_addr(rt2x00dev,
 EEPROM_MAC_ADDR_0);
      if (!is_valid_ether_addr(mac)) {
 -       
 random_ether_addr(mac);
 +       
 eth_random_addr(mac);
          EEPROM(rt2x00dev,
 MAC: %pM\n, mac);
      }
  
 diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c
 b/drivers/net/wireless/rt2x00/rt2800lib.c
 index 068276e..d857d55 100644
 --- a/drivers/net/wireless/rt2x00/rt2800lib.c
 +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
 @@ -4340,7 +4340,7 @@ int rt2800_validate_eeprom(struct
 rt2x00_dev *rt2x00dev)
   */
      mac = rt2x00_eeprom_addr(rt2x00dev,
 EEPROM_MAC_ADDR_0);
      if (!is_valid_ether_addr(mac)) {
 -       
 random_ether_addr(mac);
 +       
 eth_random_addr(mac);
          EEPROM(rt2x00dev,
 MAC: %pM\n, mac);
      }
  
 diff --git a/drivers/net/wireless/rt2x00/rt61pci.c
 b/drivers/net/wireless/rt2x00/rt61pci.c
 index ee22bd7..f322596 100644
 --- a/drivers/net/wireless/rt2x00/rt61pci.c