Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
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
References: <20220301074905.GA13071@kili>
Date:   Tue, 05 Apr 2022 10:49:05 +0300
In-Reply-To: <20220301074905.GA13071@kili> (Dan Carpenter's message of "Tue, 1
        Mar 2022 10:49:05 +0300")
Message-ID: <8735isrmvi.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

+ 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