| Message ID | 20200825171656.75836-2-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | regmap: add SoundWire 1.2 MBQ support | expand |
On Tue, Aug 25, 2020 at 12:16:53PM -0500, Pierre-Louis Bossart wrote:
> -ENOTSUPP is not a valid error code, use recommended value instead.
What makes you say this - it's what regmap uses internally for
unsupported operations?
>> -ENOTSUPP is not a valid error code, use recommended value instead. > > What makes you say this - it's what regmap uses internally for > unsupported operations? This was flagged by scripts/checkpatch.pl (must be a new addition). # ENOTSUPP is not a standard error code and should be avoided in new patches. # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP. # Similarly to ENOSYS warning a small number of false positives is expected. if (!$file && $line =~ /\bENOTSUPP\b/) { if (WARN("ENOTSUPP", "ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP\n" . $herecurr) && $fix) { $fixed[$fixlinenr] =~ s/\bENOTSUPP\b/EOPNOTSUPP/; } } I was just blindly making checkpatch happy and tried to keep regmap-sdw.c and regmap-sdw-mbq aligned.
On Tue, Aug 25, 2020 at 05:08:55PM -0500, Pierre-Louis Bossart wrote: > > > -ENOTSUPP is not a valid error code, use recommended value instead. > > What makes you say this - it's what regmap uses internally for > > unsupported operations? > This was flagged by scripts/checkpatch.pl (must be a new addition). checkpatch is broken.
On Wed, 26 Aug 2020 11:56:01 +0200, Mark Brown wrote: > > On Tue, Aug 25, 2020 at 05:08:55PM -0500, Pierre-Louis Bossart wrote: > > > > > -ENOTSUPP is not a valid error code, use recommended value instead. > > > > What makes you say this - it's what regmap uses internally for > > > unsupported operations? > > > This was flagged by scripts/checkpatch.pl (must be a new addition). > > checkpatch is broken. Heh, I'm not objecting it :) OTOH, it's also true that ENOTSUPP is no good error code if returned to user-space. Unfortunately many codes (including what I wrote) use this code mistakenly, and they can't be changed any longer... Takashi
On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > checkpatch is broken. > Heh, I'm not objecting it :) > OTOH, it's also true that ENOTSUPP is no good error code if returned > to user-space. Unfortunately many codes (including what I wrote) use > this code mistakenly, and they can't be changed any longer... It's also used internally in various places without being returned to userspace, that's what's going on here - the regmap core has some specific checks for -ENOTSUPP.
On Wed, 26 Aug 2020 12:13:01 +0200, Mark Brown wrote: > > On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > checkpatch is broken. > > > Heh, I'm not objecting it :) > > > OTOH, it's also true that ENOTSUPP is no good error code if returned > > to user-space. Unfortunately many codes (including what I wrote) use > > this code mistakenly, and they can't be changed any longer... > > It's also used internally in various places without being returned to > userspace, that's what's going on here - the regmap core has some > specific checks for -ENOTSUPP. Sure, for such an internal usage any code can be used. The question is a case like this -- where the return code might be carried to outside. Though, looking through the grep output, all callers simply return -EINVAL for any errors, so it doesn't matter much for now. BTW, there are a few callers of devm_regmap_init_sdw() checking the return value with NULL. This will crash as the function returns the error pointer, and they must be checked with IS_ERR() instead. Takashi
On 26-08-20, 12:22, Takashi Iwai wrote: > On Wed, 26 Aug 2020 12:13:01 +0200, > Mark Brown wrote: > > > > On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote: > > > Mark Brown wrote: > > > > > > checkpatch is broken. > > > > > Heh, I'm not objecting it :) > > > > > OTOH, it's also true that ENOTSUPP is no good error code if returned > > > to user-space. Unfortunately many codes (including what I wrote) use > > > this code mistakenly, and they can't be changed any longer... > > > > It's also used internally in various places without being returned to > > userspace, that's what's going on here - the regmap core has some > > specific checks for -ENOTSUPP. > > Sure, for such an internal usage any code can be used. > The question is a case like this -- where the return code might be > carried to outside. Though, looking through the grep output, all > callers simply return -EINVAL for any errors, so it doesn't matter > much for now. > > > BTW, there are a few callers of devm_regmap_init_sdw() checking the > return value with NULL. This will crash as the function returns the > error pointer, and they must be checked with IS_ERR() instead. Yes that is correct, all expect wsa codec do the incorrect thing. Will send patches for these shortly Thanks
>>>> checkpatch is broken. >> >>> Heh, I'm not objecting it :) >> >>> OTOH, it's also true that ENOTSUPP is no good error code if returned >>> to user-space. Unfortunately many codes (including what I wrote) use >>> this code mistakenly, and they can't be changed any longer... >> >> It's also used internally in various places without being returned to >> userspace, that's what's going on here - the regmap core has some >> specific checks for -ENOTSUPP. > > Sure, for such an internal usage any code can be used. > The question is a case like this -- where the return code might be > carried to outside. Though, looking through the grep output, all > callers simply return -EINVAL for any errors, so it doesn't matter > much for now. I assumed this change to -EOPNOTSUPP reflected a consensus in kernel-land, it's obviously not the case. This patch was supposed to be a trivial clean-up... So to be clear, what is the direction for existing code a) keep -ENOTSUPP as is? b) move to -EOPNOTSUPP? And what is the preference for new code?
On Wed, Aug 26, 2020 at 10:05:50AM -0500, Pierre-Louis Bossart wrote: > I assumed this change to -EOPNOTSUPP reflected a consensus in kernel-land, > it's obviously not the case. This patch was supposed to be a trivial > clean-up... No, it's just some random guy sent a patch. They've not made any perceptible effort to interact with any of the existing users. > So to be clear, what is the direction for existing code > a) keep -ENOTSUPP as is? > b) move to -EOPNOTSUPP? > And what is the preference for new code? If you want to change this you'd need to change it over the whole subsystem (if not other subsystems), including the places where the value is used.
> If you want to change this you'd need to change it over the whole > subsystem (if not other subsystems), including the places where the > value is used. ok, I'll drop this patch for now, keep -ENOTSUPP and deal with this later. The only thing I really care about for now is SoundWire MBQ support, this is the only thing gating SDCA contributions.
diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c index 50a66382d87d..89d3856f5890 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -40,14 +40,14 @@ static int regmap_sdw_config_check(const struct regmap_config *config) { /* All register are 8-bits wide as per MIPI Soundwire 1.0 Spec */ if (config->val_bits != 8) - return -ENOTSUPP; + return -EOPNOTSUPP; /* Registers are 32 bits wide */ if (config->reg_bits != 32) - return -ENOTSUPP; + return -EOPNOTSUPP; if (config->pad_bits != 0) - return -ENOTSUPP; + return -EOPNOTSUPP; return 0; }