| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <df7c735f0caeb449bbaa8a6e040ae556@codeaurora.org>
Date: Fri, 18 Jun 2021 10:17:59 -0700
From: Bhaumik Bhatt <bbhatt@...eaurora.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: linux-arm-msm@...r.kernel.org, hemantk@...eaurora.org,
jhugo@...eaurora.org, linux-kernel@...r.kernel.org,
carl.yin@...ctel.com, naveen.kumar@...ctel.com,
loic.poulain@...aro.org
Subject: Re: [PATCH v1 1/4] bus: mhi: core: Add support for processing
priority of event ring
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@...eaurora.org>
>>
>> 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@...eaurora.org>
>> Signed-off-by: Bhaumik Bhatt <bbhatt@...eaurora.org>
>> ---
>> 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.
>> 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)
>> }
>> }
>>
>> -irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>> +irqreturn_t mhi_irq_handler(int irq_number, void *priv)
>> {
>> - struct mhi_event *mhi_event = dev;
>> + struct mhi_event *mhi_event = priv;
>> struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl;
>> + struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> struct mhi_event_ctxt *er_ctxt =
>> &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index];
>> struct mhi_ring *ev_ring = &mhi_event->ring;
>> @@ -454,8 +455,20 @@ irqreturn_t mhi_irq_handler(int irq_number, void
>> *dev)
>>
>> if (mhi_dev)
>> mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
>> - } else {
>> +
>> + return IRQ_HANDLED;
>> + }
>> +
>> + switch (mhi_event->priority) {
>> + case MHI_ER_PRIORITY_HI:
>
> This could be,
>
> /* Use high priority tasklet for high priority event ring */
> if (!mhi_event->priority)
> tasklet_hi_schedule(&mhi_event->task);
> else
> tasklet_schedule(&mhi_event->task);
>
> Thanks,
> Mani
>
Yes, this works too if we keep the current non-enum approach.
>> + tasklet_hi_schedule(&mhi_event->task);
>> + break;
>> + case MHI_ER_PRIORITY_DEFAULT:
>> tasklet_schedule(&mhi_event->task);
>> + break;
>> + default:
>> + dev_err(dev, "Skip event of unknown priority\n");
>> + break;
>> }
>>
>> return IRQ_HANDLED;
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index 86cea52..25ee312 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -198,6 +198,16 @@ enum mhi_er_data_type {
>> };
>>
>> /**
>> + * enum mhi_er_priority - Event ring processing priority
>> + * @MHI_ER_PRIORITY_HI: processed by hi-priority tasklet
>> + * @MHI_ER_PRIORITY_DEFAULT: processed by regular tasklet
>> + */
>> +enum mhi_er_priority {
>> + MHI_ER_PRIORITY_HI,
>> + MHI_ER_PRIORITY_DEFAULT,
>> +};
>> +
>> +/**
>> * enum mhi_db_brst_mode - Doorbell mode
>> * @MHI_DB_BRST_DISABLE: Burst mode disable
>> * @MHI_DB_BRST_ENABLE: Burst mode enable
>> @@ -250,7 +260,7 @@ struct mhi_channel_config {
>> * @irq_moderation_ms: Delay irq for additional events to be
>> aggregated
>> * @irq: IRQ associated with this ring
>> * @channel: Dedicated channel number. U32_MAX indicates a
>> non-dedicated ring
>> - * @priority: Priority of this ring. Use 1 for now
>> + * @priority: Processing priority of this ring.
>> * @mode: Doorbell mode
>> * @data_type: Type of data this ring will process
>> * @hardware_event: This ring is associated with hardware channels
>> @@ -262,7 +272,7 @@ struct mhi_event_config {
>> u32 irq_moderation_ms;
>> u32 irq;
>> u32 channel;
>> - u32 priority;
>> + enum mhi_er_priority priority;
>> enum mhi_db_brst_mode mode;
>> enum mhi_er_data_type data_type;
>> bool hardware_event;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>
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?
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
Powered by blists - more mailing lists