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

[Pranjal Shrivastava <praan@google.com>]

On Tue, Oct 01, 2024 at 03:59:54PM -0700, Nicolin Chen wrote:
> 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.

Ahh, right! That makes more sense. I'll re-organize them like this.

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

Hmm, right or maybe ONLY have the master_name in arm_smmu_event?

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

Ack.

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

Ack. How about `read_arm_smmu_event` :) 
Unless, we wanna follow the arm_smmu_<func> convention?

> Thanks
> Nicolin

Thanks!
Praan