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

[Pranjal Shrivastava <praan@google.com>]

On Fri, Oct 18, 2024 at 06:56:14PM -0700, Nicolin Chen wrote:
> On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> > Introduce `struct arm_smmu_event` to represent event records.
> > Parse out relevant fields from raw event records for ease and
> > use the new `struct arm_smmu_event` instead.
> > 
> > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> With some nits:
> 
> > +static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> > +                                       struct arm_smmu_event *event)
> > +{
> > +       /* Pick out the good stuff */
> > +       event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> > +       event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > +       event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
> > +       event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> 
> 	event->ssid = event->ssid_valid ?
> 		      FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> 
> Or maybe would better memset(0) the event, and then:
> 	if (event->ssid_valid)
> 		event->ssid = FIELD_GET(EVTQ_0_SSID, event->raw[0]);
> 

I like the former better but I think we should also memset(event, 0)

> >  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> 
> >         do {
> > -               while (!queue_remove_raw(q, evt)) {
> > -                       u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > +               while (!queue_remove_raw(q, evt.raw)) {
> > 
> > -                       ret = arm_smmu_handle_evt(smmu, evt);
> 
> Could drop that blank line in-between as well.
> 

Ack.

> > +                       arm_smmu_get_event_from_raw(smmu, &evt);
> > +                       ret = arm_smmu_handle_evt(&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)evt.raw[i]);
>                                           ^
> Could keep the previous indentation -----|
> 

Ack.

> Nicolin

Thanks!
Pranjal