All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kalle Valo <kvalo@kernel.org>
To: Dan Carpenter <dan.carpenter@oracle.com>,
	Aloka Dixit <quic_alokad@quicinc.com>
Cc: john@phrozen.org, ath11k@lists.infradead.org,
	linux-wireless@vger.kernel.org
Subject: Re: [bug report] ath11k: add debugfs for TWT debug calls
Date: Tue, 05 Apr 2022 10:49:05 +0300	[thread overview]
Message-ID: <8735isrmvi.fsf@kernel.org> (raw)
In-Reply-To: <20220301074905.GA13071@kili> (Dan Carpenter's message of "Tue, 1 Mar 2022 10:49:05 +0300")

+ aloka

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Hello John Crispin,
>
> The patch fe98a6137d03: "ath11k: add debugfs for TWT debug calls"
> from Jan 31, 2022, leads to the following Smatch static checker
> warning:
>
> 	drivers/net/wireless/ath/ath11k/debugfs.c:1642 ath11k_debugfs_add_interface()
> 	warn: 'arvif->debugfs_twt' is an error pointer or valid
>
> drivers/net/wireless/ath/ath11k/debugfs.c
>     1637 int ath11k_debugfs_add_interface(struct ath11k_vif *arvif)
>     1638 {
>     1639         if (arvif->vif->type == NL80211_IFTYPE_AP && !arvif->debugfs_twt) {
>     1640                 arvif->debugfs_twt = debugfs_create_dir("twt",
>     1641                                                         arvif->vif->debugfs_dir);
> --> 1642                 if (!arvif->debugfs_twt || IS_ERR(arvif->debugfs_twt)) {
>     1643                         ath11k_warn(arvif->ar->ab,
>     1644                                     "failed to create directory %p\n",
>     1645                                     arvif->debugfs_twt);
>
> The debugfs_create_dir() function never returns NULL.  It's generally
> not supposed to checked for errors.  This code here looks like a
> layering violation because it's trying to check if debugfs is already
> registered.  But the clean up code just unregisters on the first call.
> Should it be ref counted or can the !arvif->debugfs_twt check be
> removed?
>
> Also if the user deliberately disabled debugfs then this prints an error
> message.
>
>     1646                         arvif->debugfs_twt = NULL;
>     1647                         return -1;

Please also use proper error values, not -1.

>     1648                 }
>     1649 
>     1650                 debugfs_create_file("add_dialog", 0200, arvif->debugfs_twt,
>     1651                                     arvif, &ath11k_fops_twt_add_dialog);
>     1652 
>     1653                 debugfs_create_file("del_dialog", 0200, arvif->debugfs_twt,
>     1654                                     arvif, &ath11k_fops_twt_del_dialog);
>     1655 
>     1656                 debugfs_create_file("pause_dialog", 0200, arvif->debugfs_twt,
>     1657                                     arvif, &ath11k_fops_twt_pause_dialog);
>     1658 
>     1659                 debugfs_create_file("resume_dialog", 0200, arvif->debugfs_twt,
>     1660                                     arvif, &ath11k_fops_twt_resume_dialog);
>     1661         }
>     1662         return 0;
>     1663 }

Aloka, you submitted this patch. Please take a look and fix the issues.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

WARNING: multiple messages have this Message-ID (diff)
From: Kalle Valo <kvalo@kernel.org>
To: Dan Carpenter <dan.carpenter@oracle.com>,
	Aloka Dixit <quic_alokad@quicinc.com>
Cc: john@phrozen.org, ath11k@lists.infradead.org,
	linux-wireless@vger.kernel.org
Subject: Re: [bug report] ath11k: add debugfs for TWT debug calls
Date: Tue, 05 Apr 2022 10:49:05 +0300	[thread overview]
Message-ID: <8735isrmvi.fsf@kernel.org> (raw)
In-Reply-To: <20220301074905.GA13071@kili> (Dan Carpenter's message of "Tue, 1 Mar 2022 10:49:05 +0300")

+ aloka

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Hello John Crispin,
>
> The patch fe98a6137d03: "ath11k: add debugfs for TWT debug calls"
> from Jan 31, 2022, leads to the following Smatch static checker
> warning:
>
> 	drivers/net/wireless/ath/ath11k/debugfs.c:1642 ath11k_debugfs_add_interface()
> 	warn: 'arvif->debugfs_twt' is an error pointer or valid
>
> drivers/net/wireless/ath/ath11k/debugfs.c
>     1637 int ath11k_debugfs_add_interface(struct ath11k_vif *arvif)
>     1638 {
>     1639         if (arvif->vif->type == NL80211_IFTYPE_AP && !arvif->debugfs_twt) {
>     1640                 arvif->debugfs_twt = debugfs_create_dir("twt",
>     1641                                                         arvif->vif->debugfs_dir);
> --> 1642                 if (!arvif->debugfs_twt || IS_ERR(arvif->debugfs_twt)) {
>     1643                         ath11k_warn(arvif->ar->ab,
>     1644                                     "failed to create directory %p\n",
>     1645                                     arvif->debugfs_twt);
>
> The debugfs_create_dir() function never returns NULL.  It's generally
> not supposed to checked for errors.  This code here looks like a
> layering violation because it's trying to check if debugfs is already
> registered.  But the clean up code just unregisters on the first call.
> Should it be ref counted or can the !arvif->debugfs_twt check be
> removed?
>
> Also if the user deliberately disabled debugfs then this prints an error
> message.
>
>     1646                         arvif->debugfs_twt = NULL;
>     1647                         return -1;

Please also use proper error values, not -1.

>     1648                 }
>     1649 
>     1650                 debugfs_create_file("add_dialog", 0200, arvif->debugfs_twt,
>     1651                                     arvif, &ath11k_fops_twt_add_dialog);
>     1652 
>     1653                 debugfs_create_file("del_dialog", 0200, arvif->debugfs_twt,
>     1654                                     arvif, &ath11k_fops_twt_del_dialog);
>     1655 
>     1656                 debugfs_create_file("pause_dialog", 0200, arvif->debugfs_twt,
>     1657                                     arvif, &ath11k_fops_twt_pause_dialog);
>     1658 
>     1659                 debugfs_create_file("resume_dialog", 0200, arvif->debugfs_twt,
>     1660                                     arvif, &ath11k_fops_twt_resume_dialog);
>     1661         }
>     1662         return 0;
>     1663 }

Aloka, you submitted this patch. Please take a look and fix the issues.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

  reply	other threads:[~2022-04-05  7:49 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-01  7:49 [bug report] ath11k: add debugfs for TWT debug calls Dan Carpenter
2022-03-01  7:49 ` Dan Carpenter
2022-04-05  7:49 ` Kalle Valo [this message]
2022-04-05  7:49   ` Kalle Valo
2022-04-07 17:32   ` Aloka Dixit
2022-04-07 17:32     ` Aloka Dixit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8735isrmvi.fsf@kernel.org \
    --to=kvalo@kernel.org \
    --cc=ath11k@lists.infradead.org \
    --cc=dan.carpenter@oracle.com \
    --cc=john@phrozen.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=quic_alokad@quicinc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.