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

[Nicolin Chen <nicolinc@nvidia.com>]

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]);

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

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

Nicolin