Re: [PATCH net-next-2.6] can: Proper ctrlmode handling for CAN devices

[Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>]

christian pellegrin wrote:
> On Wed, Jan 13, 2010 at 8:56 PM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
>> Hi Christian,
> 
> Hi,
> 
>> Could you please explain, what the "check_ctrlmode" callback is good
>> for. For me it seems useless, at a first glance. Without, also the
>> variable ctrlmode is not necessary.
> 
> It's needed to avoid unmeaningful combinations like loop-back +
> listen-only (it's quite sure you won't hear nothing and this mode
> isn't even programmable on the mcp251x for example; other could be
> more subtle, like having one-shot mode on or off doesn't make any
> difference both with loop-back or listen-only). Of course I can
> hard-code this but if we add some other fancy options with
> controller-specific behavior I'm not sure all the possible cases could
> be catch. On the other hand it's supposed that people who set ctrlmode
> more or less know what are they doing, so this test may be superflous.
> If you think so I can just eliminate it.

I see. I really don't like the extra callback. Currently, it seems
overkill to me. In principle, we could also do some checks in the device
open function, if needed.

>>> +             return -EOPNOTSUPP;
>> In another mail you mentioned, that "ENOTSUPP" does not result in a
>> useful user space error message. I checked "errno.h" of my Linux
>> distribution and there ENOTSUPP is not even defined, in contrast to
>> "EOPNOTSUPP". Hm, ENOTSUPP is used in may places in the kernel and also
>> in some CAN source files and I think we should fix that.
>>
> 
> I agree, perhaps this should be pointed out on LKML too (even if we
> risk to ignite a flame war between kernel and glibc folks ;-) )

I found some links on that subject. Obviously, there is ENOTSUP and
EOPNOTSUPP in the glibc, which are equal, but no ENOTSUPP. I tend to
replace ENOTSUPP with EOPNOTSUPP, EINVAL or ENOSYS, what ever is more
appropriate. Do you feel that EINVAL is more appropriate for the case above?

>>> +     return 0;
>>> +}
>> For me this check never fails if "priv->can.ctrlmode_supported" is set
>> properly. Or have I missed something?
>>
> 
> as I said above it catches the case when the device is put in
> loop-back and listen-only at the same time.

Let's keep it simple for the moment. In future, we may need something
more sophisticated for feature and capability handling anyhow.

Wolfgang.