Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> +{
> +       struct arm_smmu_device *smmu = evt->smmu;
> +       char title[100] = {0};
> +       char mastr[100] = {0};
> +       char addrs[100] = {0};
> +       char flags[100] = {0};
> +       char other[50] = {0};
> +
> +       if (!__ratelimit(rs))
> +               return;
> +
> +       snprintf(title, 100, "Unexpected event received: %s\n", event_str[evt->id]);
> +       snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> +                evt->master_name, evt->sid, evt->ssid);

Likely I mentioned in the cover-letter, maybe "sid.ssid:"?

> +       switch (evt->id) {
> +       case EVT_ID_TRANSLATION_FAULT:
> +       case EVT_ID_ADDR_SIZE_FAULT:
> +       case EVT_ID_ACCESS_FAULT:
> +       case EVT_ID_PERMISSION_FAULT:
> +               snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> +               snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> +               snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> +                        evt->privileged ? "Priv | " : "Unpriv | ",
> +                        evt->instruction ? "Inst | " : "Data | ",
> +                        evt->read ? "Read | " : "Write | ",
> +                        evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> +                        evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");

Should the last one be "TTD Read |" : "TTD Write|"?
Otherwise, it would be "S2 || TTD Read" combined.

>  static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
>                                         struct arm_smmu_event *event)
>  {
> +       struct arm_smmu_master *master;
> +
>         /* 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]);
> @@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
>         event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
>         event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
>         event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> +       event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
> +       event->ttrnw_valid = false;
>         event->smmu = smmu;
> +       event->dev = NULL;
> +
> +       if (event->id == EVT_ID_PERMISSION_FAULT)
> +               event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
> +
> +       mutex_lock(&smmu->streams_mutex);
> +       master = arm_smmu_find_master(smmu, event->sid);
> +       if (master)
> +               event->dev = get_device(master->dev);

Here, get_device is called upon a valid master...

>  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>  {
> -       int i, ret;
>         struct arm_smmu_event evt;
>         struct arm_smmu_device *smmu = dev;
>         struct arm_smmu_queue *q = &smmu->evtq.q;
> @@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>                 while (!queue_remove_raw(q, evt.raw)) {
> 
>                         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", evt.id);
> -                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> -                               dev_info(smmu->dev, "\t0x%016llx\n",
> -                                       (unsigned long long)evt.raw[i]);
> +                       if (arm_smmu_handle_evt(&evt))
> +                               arm_smmu_dump_event(&evt, &rs);
> 
> +                       put_device(evt.dev);

then, here it puts unconditionally.

Maybe we do need a memset(0) to the event, then here
			if (evet.dev)
				put_device(evt.dev);

Thanks
Nicolin