Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Nicolin Chen <nicolinc@nvidia.com>]

On Tue, Oct 01, 2024 at 09:02:42PM +0000, Pranjal Shrivastava wrote:
> On Mon, Sep 30, 2024 at 12:32:21PM -0700, Nicolin Chen wrote:
> > On Sat, Sep 28, 2024 at 12:51:43AM +0000, Pranjal Shrivastava wrote:
> > >         mutex_lock(&smmu->streams_mutex);
> > > -       master = arm_smmu_find_master(smmu, sid);
> > > -       if (!master) {
> > > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > > +       if (!event->master) {
> > >                 ret = -EINVAL;
> > > +               event->master_name = "(unassigned sid)";
> > >                 goto out_unlock;
> > >         }
> >
> > The PATCH-1 already did arm_smmu_find_master() to event->master in
> > arm_smmu_get_evt_info()?
> >
> > Maybe we still need a wider mutex to lock arm_smmu_get_evt_info and
> > arm_smmu_handle_evt.
> >
> 
> Yea, we did, I'm wondering if we really need to read the master_name in
> arm_smmu_get_evt_info ? I mean, we can find and populate the name here
> itself under the safety of this mutex and entirely remove that lock from
> arm_smmu_get_evt_info as we anyway will dump the event after this line.

We could. I guess the two patches organized in the slightly odd
way that the arm_smmu_dump_event() gets moved around in the 2nd
patch, so things look a bit redundant after all.

I think we could do two patches like:
PATCH-1: Add struct arm_smmu_event and swap all FIELD_GETs with
         the structure members.
PATCH-2: Add arm_smmu_dump_event() and put in arm_smmu_handle_evt
         directly.

The master and master_name are not that necessary to be in the
arm_smmu_event. I would keep both of them as local variables in
arm_smmu_handle_evt and pass master_name in to dump().

The dev_name() returns "const char *init_name", so it'd likely
be safe to put the arm_smmu_dump_event(&event, master_name) call
outside the mutex.

> I'm not too sure if we should hold this lock for the entire duration of
> get_evt_info and handle_evt ?

It should be fine for being a mutex. Yet, shrinking its scope is
always optimal.

> Also, shall we rename it to `arm_smmu_read_evt_info` ?

I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
no strong feeling about that.

Thanks
Nicolin