Re: [PATCH v2 2/3] usb: typec: ucsi: Update connector cap and status - Abhishek Pandit-Subedi

From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Abhishek Pandit-Subedi <abhishekpandit@google.com>,
	 Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	linux-usb@vger.kernel.org,  jthies@google.com,
	pmalani@chromium.org,
	 Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	 Rajaram Regupathy <rajaram.regupathy@intel.com>,
	Saranya Gopal <saranya.gopal@intel.com>,
	 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] usb: typec: ucsi: Update connector cap and status
Date: Fri, 26 Jan 2024 10:08:16 -0800	[thread overview]
Message-ID: <CANFp7mVPahm+6MjD_+MWMNUz=RViNh777h=Q2dW0UVVDK6dA0A@mail.gmail.com> (raw)
In-Reply-To: <2024012555-nuclear-chummy-6079@gregkh>

On Thu, Jan 25, 2024 at 5:50 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 25, 2024 at 04:21:47PM -0800, Abhishek Pandit-Subedi wrote:
> > On Thu, Jan 25, 2024 at 3:03 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jan 24, 2024 at 04:44:53PM -0800, Abhishek Pandit-Subedi wrote:
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > > > index bec920fa6b8a..94b373378f63 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi.h
> > > > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > > > @@ -3,6 +3,7 @@
> > > >  #ifndef __DRIVER_USB_TYPEC_UCSI_H
> > > >  #define __DRIVER_USB_TYPEC_UCSI_H
> > > >
> > > > +#include <asm-generic/unaligned.h>
> > >
> > > Do you really need to include a asm/ include file?  This feels very
> > > wrong.
> >
> > I didn't see any header in include/linux that already had these
> > unaligned access functions so I opted to include
> > asm-generic/unaligned.h. Is there a reason not to use an asm/ include
> > file?
>
> Yes, you should never need to include a asm/ file, unless you are
> arch-specific code.
>
> But the big issue is that you don't really need this, right?

The UCSI struct definitions have lots of unaligned bit ranges (and I
will be refactoring <linux/bitfield.h> to support this but that's
coming later). As an example, the GET_CONNECTOR_STATUS data structure
has unaligned fields from bit 88-145.
Rather than define my own macro, it was suggested I use the
get_unaligned_le32 functions (see
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5195032/3..4/drivers/usb/typec/ucsi/ucsi.h#b183).

I did a quick ripgrep on the drivers folder -- it looks like the "You
should never need to include a asm/ file unless you are arch specific"
isn't being followed for this file:
  $ (cd drivers && rg -g '*.h' "unaligned\.h" -l) | wc -l
  22

The unaligned access functions (get_unaligned_le16,
get_unaligned_le32, etc) are really useful and widely used. Maybe they
SHOULD be exposed from a <linux/unaligned.h> since they are so useful?
I can send a follow-on patch that creates <linux/unaligned.h> (that
simply just includes <asm/unaligned.h>) and moves all includes of
<asm/unaligned.h> outside of "arch" to the linux header instead (this
will also create a checkpatch warning now as you are expecting).

>
> > > It's also in the wrong place, AND why "asm-generic"?  That also feels
> > > wrong.
> >
> > asm-generic is definitely wrong; I misunderstood how these headers are
> > supposed to be used (should be just asm/unaligned.h).
>
> Why?  What are you requiring this .h file for?
>
> > For ordering, I took a look at some other files and it looks like
> > <asm/...> goes below the <linux/...> includes. This also probably
> > deserves documenting in the style guide.
>
> It is somewhere, checkpatch should complain about it.

Checkpatch only complains if there exists a <linux/foo.h> and you call
<asm/foo.h>: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?h=v6.8-rc1#n5882
That's the only relevant check I found when searched for "asm" in checkpatch.pl

>
> thanks,
>
> greg k-h