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

[Nicolin Chen <nicolinc@nvidia.com>]

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, ...

> @@ -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.

>         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.

> +                       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.

> 
>                         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;
?

Thanks
Nicolin