On Fri, Jun 18, 2021 at 10:17:59AM -0700, Bhaumik Bhatt wrote:
> Hi Mani,
>
> On 2021-06-18 12:03 AM, Manivannan Sadhasivam wrote:
> > On Thu, Jun 17, 2021 at 02:30:32PM -0700, Bhaumik Bhatt wrote:
> > > From: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> > >
> > > Event ring priorities are currently set to 1 and are unused.
> > > Default processing priority for event rings is set to regular
> > > tasklet. Controllers can choose to use high priority tasklet
> > > scheduling for certain event rings critical for processing such
> > > as ones transporting control information if they wish to avoid
> > > with system scheduling delays for those packets. In order to
> > > support these use cases, allow controllers to set event ring
> > > priority to high. This patch only adds support and does not
> > > enable usage of these priorities.
> > >
> > > Signed-off-by: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> > > Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> > > ---
> > > drivers/bus/mhi/core/internal.h | 2 +-
> > > drivers/bus/mhi/core/main.c | 19 ++++++++++++++++---
> > > include/linux/mhi.h | 14 ++++++++++++--
> > > 3 files changed, 29 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/bus/mhi/core/internal.h
> > > b/drivers/bus/mhi/core/internal.h
> > > index 672052f..666e102 100644
> > > --- a/drivers/bus/mhi/core/internal.h
> > > +++ b/drivers/bus/mhi/core/internal.h
> > > @@ -535,7 +535,7 @@ struct mhi_event {
> > > u32 intmod;
> > > u32 irq;
> > > int chan; /* this event ring is dedicated to a channel (optional) */
> > > - u32 priority;
> > > + enum mhi_er_priority priority;
> >
> > Instead of using enum for priorities, can we just make use of the
> > existing "priority" field? Since the existing controllers set it to "1",
> > can we use "0" as the high priority?
> >
> > This way we don't need to change the controller drivers.
> >
> I agree but the reasons to do the enum approach was to allow for future
> expansion of the handling if it becomes necessary and provide clarity for
> the field.
>
> I can always do it this way for now and we can have the enum for another
> time but would prefer updating what we have now.
Yeah, let's deal with it later once the necessity arises.
> > > enum mhi_er_data_type data_type;
> > > struct mhi_ring ring;
> > > struct db_cfg db_cfg;
> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > index 8ac73f9..bfc9776 100644
> > > --- a/drivers/bus/mhi/core/main.c
> > > +++ b/drivers/bus/mhi/core/main.c
> > > @@ -425,10 +425,11 @@ void mhi_create_devices(struct mhi_controller
> > > *mhi_cntrl)
> > > }
> > > }
> > >
[...]
> Existing controllers would be fine.
>
> Do you think we have a problem if a new controller blindly inputs a "0" in
> the priority not knowing the impact of it?
>
We should document it in the kernel doc for the struct field and that
should be enough. We can't do much if people doesn't read the doc ;)
Thanks,
Mani
> If you give me a go ahead, I can make these changes in v2 and leave the enum
> stuff out.
>
> Thanks,
> Bhaumik
> ---
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
