| Message ID | e4556e95a5935f591fdff67c6fb2f7ef5c7fd04f.1490914618.git.joe@perches.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 169345d40d0fa05c4fdcec67835b008d24cfcf26 |
| Delegated to: | Kalle Valo |
| Headers | show |
On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <joe@perches.com> wrote: > Fix fallout too. > > Signed-off-by: Joe Perches <joe@perches.com> > --- > drivers/net/wireless/ath/ath6kl/debug.h | 2 ++ > drivers/net/wireless/ath/ath6kl/htc_pipe.c | 2 +- > drivers/net/wireless/ath/ath6kl/wmi.c | 2 +- > 3 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath6kl/debug.h b/drivers/net/wireless/ath/ath6kl/debug.h > index 0614393dd7ae..94297572914f 100644 > --- a/drivers/net/wireless/ath/ath6kl/debug.h > +++ b/drivers/net/wireless/ath/ath6kl/debug.h > @@ -63,6 +63,7 @@ int ath6kl_read_tgt_stats(struct ath6kl *ar, struct ath6kl_vif *vif); > > #ifdef CONFIG_ATH6KL_DEBUG > > +__printf(2, 3) > void ath6kl_dbg(enum ATH6K_DEBUG_MASK mask, const char *fmt, ...); > void ath6kl_dbg_dump(enum ATH6K_DEBUG_MASK mask, > const char *msg, const char *prefix, > @@ -83,6 +84,7 @@ int ath6kl_debug_init_fs(struct ath6kl *ar); > void ath6kl_debug_cleanup(struct ath6kl *ar); > > #else > +__printf(2, 3) > static inline void ath6kl_dbg(enum ATH6K_DEBUG_MASK dbg_mask, > const char *fmt, ...) > { My only question is why bother doing a format check on something that's going to be compiled out anyway? I suppose the only harm is a tiny extra bit of compile time due to the check and I'm sure that's measured in micro-seconds on full development systems, but if we do it everywhere those tiny bits of time would eventually add up. Admittedly it's a comment that probably isn't worth redoing the commit over. I guess I'm bringing up the point more discuss the question: "Should we add the printf format verification on the clauses that get compiled out?" So, it looks good to me as is, or if you feel like making the change I'm suggesting, that's fine too. And it builds and runs on my platforms. Reviewed-by: Steve deRosier <derosier@gmail.com> - Steve
On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote: > On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <joe@perches.com> wrote: > > Fix fallout too. [] > My only question is why bother doing a format check on something > that's going to be compiled out anyway? To avoid introducing defects when writing new code and not using the debugging code path.
On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches <joe@perches.com> wrote: > On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote: >> On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <joe@perches.com> wrote: >> > Fix fallout too. > [] >> My only question is why bother doing a format check on something >> that's going to be compiled out anyway? > > To avoid introducing defects when writing new code > and not using the debugging code path. > Fair enough. And I totally agree with the defensive programming here in that case and feel it's worth the tradeoff (if indeed there really is any cost, I'm unsure what gcc actually does in this instance). For sake of discussion though - shouldn't anything not using the debug code path in this case always be of the form that compiles out? ie would be empty functions intended here just to make compilation work and the code that depends on it simpler? Thus, there really should never be a risk of introducing said defects. If any "real" code were put in that else clause, that'd be a big red-flag in the review of said hypothetical patch. Thanks, - Steve
On Fri, 2017-03-31 at 10:34 -0700, Steve deRosier wrote: > On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches <joe@perches.com> wrote: > > On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote: > > > On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <joe@perches.com> wrote: > > > > Fix fallout too. > > > > [] > > > My only question is why bother doing a format check on something > > > that's going to be compiled out anyway? > > > > To avoid introducing defects when writing new code > > and not using the debugging code path. > > > > Fair enough. And I totally agree with the defensive programming here > in that case and feel it's worth the tradeoff (if indeed there really > is any cost, I'm unsure what gcc actually does in this instance). > > For sake of discussion though - shouldn't anything not using the debug > code path in this case always be of the form that compiles out? ie > would be empty functions intended here just to make compilation work > and the code that depends on it simpler? Thus, there really should > never be a risk of introducing said defects. If any "real" code were > put in that else clause, that'd be a big red-flag in the review of > said hypothetical patch. Generically, all debugging forms should strive to avoid side-effects. For instance, look at no_printk/pr_debug in the #ifndef DEBUG paths. It uses if (0) to avoid compilation of arguments that might be function calls or volatile accesses and so might have side-effects altogether. include/linux/printk.h-/* include/linux/printk.h- * Dummy printk for disabled debugging statements to use whilst maintaining include/linux/printk.h- * gcc's format checking. include/linux/printk.h- */ include/linux/printk.h:#define no_printk(fmt, ...) \ include/linux/printk.h-({ \ include/linux/printk.h- do { \ include/linux/printk.h- if (0) \ include/linux/printk.h- printk(fmt, ##__VA_ARGS__); \ include/linux/printk.h- } while (0); \ include/linux/printk.h- 0; \ include/linux/printk.h-}) i
On Fri, Mar 31, 2017 at 10:45 AM, Joe Perches <joe@perches.com> wrote: > On Fri, 2017-03-31 at 10:34 -0700, Steve deRosier wrote: >> On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches <joe@perches.com> wrote: >> > On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote: >> > > On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <joe@perches.com> wrote: >> > > > Fix fallout too. >> > >> > [] >> > > My only question is why bother doing a format check on something >> > > that's going to be compiled out anyway? >> > >> > To avoid introducing defects when writing new code >> > and not using the debugging code path. >> > >> >> Fair enough. And I totally agree with the defensive programming here >> in that case and feel it's worth the tradeoff (if indeed there really >> is any cost, I'm unsure what gcc actually does in this instance). >> >> For sake of discussion though - shouldn't anything not using the debug >> code path in this case always be of the form that compiles out? ie >> would be empty functions intended here just to make compilation work >> and the code that depends on it simpler? Thus, there really should >> never be a risk of introducing said defects. If any "real" code were >> put in that else clause, that'd be a big red-flag in the review of >> said hypothetical patch. > > Generically, all debugging forms should strive to avoid side-effects. > Yes. Of course. Lightbulb. I wasn't even thinking of the fact someone could load the printf arguments with code that might have side-effects instead of simple variables to print. I never do it for obvious reasons, but I could see it happening. Thanks for spending the time going back and forth with me about it. Thanks, - Steve
Joe Perches <joe@perches.com> wrote: > Fix fallout too. > > Signed-off-by: Joe Perches <joe@perches.com> > Reviewed-by: Steve deRosier <derosier@gmail.com> Patch applied to ath-next branch of ath.git, thanks. 169345d40d0f ath6kl: add __printf verification to ath6kl_dbg
diff --git a/drivers/net/wireless/ath/ath6kl/debug.h b/drivers/net/wireless/ath/ath6kl/debug.h index 0614393dd7ae..94297572914f 100644 --- a/drivers/net/wireless/ath/ath6kl/debug.h +++ b/drivers/net/wireless/ath/ath6kl/debug.h @@ -63,6 +63,7 @@ int ath6kl_read_tgt_stats(struct ath6kl *ar, struct ath6kl_vif *vif); #ifdef CONFIG_ATH6KL_DEBUG +__printf(2, 3) void ath6kl_dbg(enum ATH6K_DEBUG_MASK mask, const char *fmt, ...); void ath6kl_dbg_dump(enum ATH6K_DEBUG_MASK mask, const char *msg, const char *prefix, @@ -83,6 +84,7 @@ int ath6kl_debug_init_fs(struct ath6kl *ar); void ath6kl_debug_cleanup(struct ath6kl *ar); #else +__printf(2, 3) static inline void ath6kl_dbg(enum ATH6K_DEBUG_MASK dbg_mask, const char *fmt, ...) { diff --git a/drivers/net/wireless/ath/ath6kl/htc_pipe.c b/drivers/net/wireless/ath/ath6kl/htc_pipe.c index ca1a18c86c0d..d127a08d60df 100644 --- a/drivers/net/wireless/ath/ath6kl/htc_pipe.c +++ b/drivers/net/wireless/ath/ath6kl/htc_pipe.c @@ -995,7 +995,7 @@ static int ath6kl_htc_pipe_rx_complete(struct ath6kl *ar, struct sk_buff *skb, if (netlen < (payload_len + HTC_HDR_LENGTH)) { ath6kl_dbg(ATH6KL_DBG_HTC, - "HTC Rx: insufficient length, got:%d expected =%u\n", + "HTC Rx: insufficient length, got:%d expected =%zu\n", netlen, payload_len + HTC_HDR_LENGTH); status = -EINVAL; goto free_skb; diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c index 84a6d12c3f8a..a082de81ec4c 100644 --- a/drivers/net/wireless/ath/ath6kl/wmi.c +++ b/drivers/net/wireless/ath/ath6kl/wmi.c @@ -1596,7 +1596,7 @@ static int ath6kl_wmi_txe_notify_event_rx(struct wmi *wmi, u8 *datap, int len, rate = le32_to_cpu(ev->rate); pkts = le32_to_cpu(ev->pkts); - ath6kl_dbg(ATH6KL_DBG_WMI, "TXE notify event: peer %pM rate %d% pkts %d intvl %ds\n", + ath6kl_dbg(ATH6KL_DBG_WMI, "TXE notify event: peer %pM rate %d%% pkts %d intvl %ds\n", vif->bssid, rate, pkts, vif->txe_intvl); cfg80211_cqm_txe_notify(vif->ndev, vif->bssid, pkts,
Fix fallout too. Signed-off-by: Joe Perches <joe@perches.com> --- drivers/net/wireless/ath/ath6kl/debug.h | 2 ++ drivers/net/wireless/ath/ath6kl/htc_pipe.c | 2 +- drivers/net/wireless/ath/ath6kl/wmi.c | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-)