Search Linux Wireless

Re: [PATCH v2] ath11k: remove error checking for debugfs_create_file()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jeff,

On Tue, Oct 29, 2024 at 1:37 AM Jeff Johnson <quic_jjohnson@xxxxxxxxxxx> wrote:
>
> On 10/28/2024 7:02 AM, Kalle Valo wrote:
> > Zhen Lei <thunder.leizhen@xxxxxxxxxx> writes:
> >
> >> Driver ath11k can work fine even if the debugfs files fail to be created.
> >> Therefore, the return value check of debugfs_create_file() should be
> >> ignored, as it says.
> >>
> >> Suggested-by: Jeff Johnson <quic_jjohnson@xxxxxxxxxxx>
> >> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
> >
> > Are you just guessing or did you confirm on a real device that ath11k
> > spectral really works without debugfs?
>
> The debugfs_create_file() documentation tells us:
>  * NOTE: it's expected that most callers should _ignore_ the errors returned
>  * by this function. Other debugfs functions handle the fact that the "dentry"
>  * passed to them could be an error and they don't crash in that case.
>  * Drivers should generally work fine even if debugfs fails to init anyway.
>
> The caveat is that any driver functionality that relies upon debugfs obviously
> won't work if the underlying file isn't created. Hence the language that the
> driver "should generally work fine" since all functionality that isn't tied to
> debugfs will still be available.

The big question for me is this:

> Since the relayfs functionality that spectral scan uses is dependent upon
> debugfs

Why?

This seems to go against the general guidance that debugfs should
essentially be "fire and forget" and return values shouldn't be
checked.

IMHO it comes down to one of two outcomes here:

1. Spectral scan isn't necessary for normal operation so we shouldn't
initialise that functionality if we can't use debugfs, which violates
the "don't check return values" guidance
2. We should break that dependency

I can envisage a lot of people making the sensible assumption that a
non-debug kernel doesn't need debugfs and therefore disabling it on
their thing, but also it seems to be enabled everywhere I have access
to: Debian Stable, WSL and Bazzite. So I could be completely missing
the point here.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@xxxxxxxxx
Profile: http://www.google.com/profiles/julian.calaby/





[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux