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

[Pranjal Shrivastava <praan@google.com>]

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:
> >  /* IRQ and event handlers */
> > -static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > +static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, struct arm_smmu_event *event)
> 
> Looks like we could stuff an smmu pointer in the event:
> 
> struct arm_smmu_event {
> +	struct arm_smmu_device *smmu;
> 	struct arm_smmu_master		*master;
> 	const char			*master_name;
> 
> Then both this arm_smmu_handle_evt and arm_smmu_dump_event can be
> slightly cleaner.
> 

Makes sense, we can read that in the get_evt_info.

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

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

Also, shall we rename it to `arm_smmu_read_evt_info` ?

> Thanks
> Nicolin

Thanks,
Pranjal