Re: [PATCH v5 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava <praan@google.com>]

On Wed, Nov 20, 2024 at 09:56:08AM +0000, Will Deacon wrote:
> On Tue, Nov 19, 2024 at 09:27:47AM +0000, Pranjal Shrivastava wrote:
> > On Tue, Nov 19, 2024 at 09:03:08AM +0000, Will Deacon wrote:
> > > On Mon, Nov 18, 2024 at 12:24:42PM -0400, Jason Gunthorpe wrote:
> > > > On Fri, Nov 15, 2024 at 01:34:12PM -0800, Nicolin Chen wrote:
> > > > > On Fri, Nov 15, 2024 at 01:13:36PM +0000, Pranjal Shrivastava wrote:
> > > > > > On Tue, Nov 12, 2024 at 03:30:58PM -0800, Nicolin Chen wrote:
> > > > > > > On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> > > > > > > > +struct arm_smmu_event {
> > > > > > > > +       u8                              stall           : 1,
> > > > > > > > +                                       ssv             : 1,
> > > > > > > > +                                       privileged      : 1,
> > > > > > > > +                                       instruction     : 1,
> > > > > > > > +                                       s2              : 1,
> > > > > > > > +                                       read            : 1;
> > > > > > > 
> > > > > > > Feels odd to do so...should all of them be:
> > > > > > > 	u8			name : 1;
> > > > > > > ?
> > > > > > 
> > > > > > Hmmm, do you mean something like the following?
> > > > > 
> > > > > Never mind. I see amd_iommu_types.h does something just like that.
> > > > 
> > > > I certainly expect to see the u8 repeated and not use , for members of
> > > > a structure. Also don't add excessive horizonal alignment.. C code is
> > > > not excel. :)
> > > 
> > > I think the comma syntax is fine (there's precedence for it and it works),
> > > but agreed on removing some of the tabs.
> > 
> > Ohh alright. I'm assuming both of you are referring to the tabs before
> > the colon ':' ? 
> > 
> > As in, is the following fine?
> > 
> > struct arm_smmu_event {
> >        u8		stall : 1,
> > 			ssv : 1,
> > 			privileged : 1,
> > 			instruction : 1,
> > 			s2 : 1,
> > 			read : 1;
> > 	.....
> 
> I was actually thinking of the whitespace between the 'u8' and the field
> names. But this is silly nit-picking. Please just follow something like
> the use of bitfields in 'struct perf_event_attr', as that's a readable
> example of recent-ish in-tree code.
> 
> Ta,

Ack. Thanks for the reference, I'll fix it!

> 
> Will

Thanks,
Praan