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

[Pranjal Shrivastava <praan@google.com>]

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:
> > +static void arm_smmu_decode_event(u64 *raw, struct arm_smmu_event *event)
> ..
> > +static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
> > +                              struct arm_smmu_event *event)
> 
> s/arm_smmu_handle_evt/arm_smmu_handle_event
> 
> Also, given that these two define "struct arm_smmu_event *event"
> nicely, ...
> 

Ack.

> > @@ -1820,25 +1836,26 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >  {
> >         int i, ret;
> > +       u64 raw_evt[EVTQ_ENT_DWORDS];
> > +       struct arm_smmu_event evt = {0};
> 
> I think we can keep the existing "evt" for raw and simply add:
> 	struct arm_smmu_event event = { };
> 
> So the patch could look cleaner, less the "s/evt/raw_evt" part.
> 

Ack. Makes sense. Will rename accordingly.

> >         struct arm_smmu_device *smmu = dev;
> >         struct arm_smmu_queue *q = &smmu->evtq.q;
> >         struct arm_smmu_ll_queue *llq = &q->llq;
> >         static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> >                                       DEFAULT_RATELIMIT_BURST);
> > -       u64 evt[EVTQ_ENT_DWORDS];
> > 
> >         do {
> > -               while (!queue_remove_raw(q, evt)) {
> > -                       u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > +               while (!queue_remove_raw(q, raw_evt)) {
> > 
> > -                       ret = arm_smmu_handle_evt(smmu, evt);
> > +                       arm_smmu_decode_event(raw_evt, &evt);
> 
> Drop the empty line in-between please.

Ack.

> 
> > +                       ret = arm_smmu_handle_evt(smmu, &evt);
> >                         if (!ret || !__ratelimit(&rs))
> >                                 continue;
> > 
> > -                       dev_info(smmu->dev, "event 0x%02x received:\n", id);
> > -                       for (i = 0; i < ARRAY_SIZE(evt); ++i)
> > +                       dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > +                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> >                                 dev_info(smmu->dev, "\t0x%016llx\n",
> > -                                        (unsigned long long)evt[i]);
> > +                                       (unsigned long long)raw_evt[i]);
> 
> Please keep the previous indentations.

Ack

>
> > 
> >                         cond_resched();
> >                 }
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index 1e9952ca989f..abb543d987f6 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -771,6 +771,22 @@ struct arm_smmu_stream {
> >         struct rb_node                  node;
> >  };
> > 
> > +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?

struct arm_smmu_event {
	u8				stall           : 1,
	u8				ssv             : 1,
	u8				privileged      : 1,
	u8				instruction     : 1,
	u8				s2              : 1,
	u8				read            : 1;
}

> 
> Thanks
> Nicolin

Thanks,
Praan