| Message ID | 20200722095740.28560-17-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SDEI client driver | expand |
On Wed, 22 Jul 2020 19:57:39 +1000 Gavin Shan <gshan@redhat.com> wrote: > This retrieves the event signaled property when it's created for the > first time. The property will be needed when SDEI virtualization is > supported. > > Signed-off-by: Gavin Shan <gshan@redhat.com> These last two patches are probably fine but hard to tell without a user. Jonathan > --- > drivers/firmware/arm_sdei.c | 6 ++++++ > include/linux/arm_sdei.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c > index cf10fec57f2a..7518d3febf53 100644 > --- a/drivers/firmware/arm_sdei.c > +++ b/drivers/firmware/arm_sdei.c > @@ -204,6 +204,12 @@ static struct sdei_event *sdei_event_create(u32 event_num, > goto fail; > event->type = result; > > + err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_SIGNALED, > + &result); > + if (err) > + goto fail; > + event->signaled = result; > + > if (event->type == SDEI_EVENT_TYPE_SHARED) { > reg = kzalloc(sizeof(*reg), GFP_KERNEL); > if (!reg) { > diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h > index 11af6410dd52..7f3ed7e4b680 100644 > --- a/include/linux/arm_sdei.h > +++ b/include/linux/arm_sdei.h > @@ -51,6 +51,7 @@ struct sdei_event { > u32 event_num; > u8 type; > u8 priority; > + u8 signaled; > > /* This pointer is handed to firmware as the event argument. */ > union {
Hi Jonathan, On 7/24/20 1:24 AM, Jonathan Cameron wrote: > On Wed, 22 Jul 2020 19:57:39 +1000 > Gavin Shan <gshan@redhat.com> wrote: > >> This retrieves the event signaled property when it's created for the >> first time. The property will be needed when SDEI virtualization is >> supported. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> > > These last two patches are probably fine but hard to tell without a user. > Good question. Let me explain the background and please let me know if you have more questions. SDEI was suggested by Marc to deliver the notification during the asynchronous page fault, so that the process can be rescheduled in guest. Unfortunately, we don't have SDEI (or virtualized SDEI) supported yet. So the additional event information is needed when SDEI virtualization is supported. The code of SDEI virtualization can be checked out from github: https://github.com/gwshan/linux/tree/sdei (branch: "sdei") >> --- >> drivers/firmware/arm_sdei.c | 6 ++++++ >> include/linux/arm_sdei.h | 1 + >> 2 files changed, 7 insertions(+) >> >> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c >> index cf10fec57f2a..7518d3febf53 100644 >> --- a/drivers/firmware/arm_sdei.c >> +++ b/drivers/firmware/arm_sdei.c >> @@ -204,6 +204,12 @@ static struct sdei_event *sdei_event_create(u32 event_num, >> goto fail; >> event->type = result; >> >> + err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_SIGNALED, >> + &result); >> + if (err) >> + goto fail; >> + event->signaled = result; >> + >> if (event->type == SDEI_EVENT_TYPE_SHARED) { >> reg = kzalloc(sizeof(*reg), GFP_KERNEL); >> if (!reg) { >> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h >> index 11af6410dd52..7f3ed7e4b680 100644 >> --- a/include/linux/arm_sdei.h >> +++ b/include/linux/arm_sdei.h >> @@ -51,6 +51,7 @@ struct sdei_event { >> u32 event_num; >> u8 type; >> u8 priority; >> + u8 signaled; >> >> /* This pointer is handed to firmware as the event argument. */ >> union { Thanks, Gavin
On Mon, 27 Jul 2020 10:53:27 +1000 Gavin Shan <gshan@redhat.com> wrote: > Hi Jonathan, > > On 7/24/20 1:24 AM, Jonathan Cameron wrote: > > On Wed, 22 Jul 2020 19:57:39 +1000 > > Gavin Shan <gshan@redhat.com> wrote: > > > >> This retrieves the event signaled property when it's created for the > >> first time. The property will be needed when SDEI virtualization is > >> supported. > >> > >> Signed-off-by: Gavin Shan <gshan@redhat.com> > > > > These last two patches are probably fine but hard to tell without a user. > > > > Good question. Let me explain the background and please let me know > if you have more questions. SDEI was suggested by Marc to deliver > the notification during the asynchronous page fault, so that the > process can be rescheduled in guest. Unfortunately, we don't have > SDEI (or virtualized SDEI) supported yet. So the additional event > information is needed when SDEI virtualization is supported. > > The code of SDEI virtualization can be checked out from github: > > https://github.com/gwshan/linux/tree/sdei (branch: "sdei") Thanks. I'd be tempted to move these two patches to the next series that includes the users. I forgot to say, I'm fine with all the patches I didn't comment on. Jonathan > > > >> --- > >> drivers/firmware/arm_sdei.c | 6 ++++++ > >> include/linux/arm_sdei.h | 1 + > >> 2 files changed, 7 insertions(+) > >> > >> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c > >> index cf10fec57f2a..7518d3febf53 100644 > >> --- a/drivers/firmware/arm_sdei.c > >> +++ b/drivers/firmware/arm_sdei.c > >> @@ -204,6 +204,12 @@ static struct sdei_event *sdei_event_create(u32 event_num, > >> goto fail; > >> event->type = result; > >> > >> + err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_SIGNALED, > >> + &result); > >> + if (err) > >> + goto fail; > >> + event->signaled = result; > >> + > >> if (event->type == SDEI_EVENT_TYPE_SHARED) { > >> reg = kzalloc(sizeof(*reg), GFP_KERNEL); > >> if (!reg) { > >> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h > >> index 11af6410dd52..7f3ed7e4b680 100644 > >> --- a/include/linux/arm_sdei.h > >> +++ b/include/linux/arm_sdei.h > >> @@ -51,6 +51,7 @@ struct sdei_event { > >> u32 event_num; > >> u8 type; > >> u8 priority; > >> + u8 signaled; > >> > >> /* This pointer is handed to firmware as the event argument. */ > >> union { > > Thanks, > Gavin >
Hi Jonathan, On 7/27/20 7:04 PM, Jonathan Cameron wrote: > On Mon, 27 Jul 2020 10:53:27 +1000 > Gavin Shan <gshan@redhat.com> wrote: >> On 7/24/20 1:24 AM, Jonathan Cameron wrote: >>> On Wed, 22 Jul 2020 19:57:39 +1000 >>> Gavin Shan <gshan@redhat.com> wrote: >>> >>>> This retrieves the event signaled property when it's created for the >>>> first time. The property will be needed when SDEI virtualization is >>>> supported. >>>> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>> >>> These last two patches are probably fine but hard to tell without a user. >>> >> >> Good question. Let me explain the background and please let me know >> if you have more questions. SDEI was suggested by Marc to deliver >> the notification during the asynchronous page fault, so that the >> process can be rescheduled in guest. Unfortunately, we don't have >> SDEI (or virtualized SDEI) supported yet. So the additional event >> information is needed when SDEI virtualization is supported. >> >> The code of SDEI virtualization can be checked out from github: >> >> https://github.com/gwshan/linux/tree/sdei (branch: "sdei") > Thanks. > > I'd be tempted to move these two patches to the next series > that includes the users. > > I forgot to say, I'm fine with all the patches I didn't comment on. > Yes, it's fine to move the last two patches to where we need them. Thanks for your review and comments. May I have your reviewed-by on those patches you didn't comment on? I would like to pick the reviwed-by in v3 :) >> >>>> --- >>>> drivers/firmware/arm_sdei.c | 6 ++++++ >>>> include/linux/arm_sdei.h | 1 + >>>> 2 files changed, 7 insertions(+) >>>> >>>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c >>>> index cf10fec57f2a..7518d3febf53 100644 >>>> --- a/drivers/firmware/arm_sdei.c >>>> +++ b/drivers/firmware/arm_sdei.c >>>> @@ -204,6 +204,12 @@ static struct sdei_event *sdei_event_create(u32 event_num, >>>> goto fail; >>>> event->type = result; >>>> >>>> + err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_SIGNALED, >>>> + &result); >>>> + if (err) >>>> + goto fail; >>>> + event->signaled = result; >>>> + >>>> if (event->type == SDEI_EVENT_TYPE_SHARED) { >>>> reg = kzalloc(sizeof(*reg), GFP_KERNEL); >>>> if (!reg) { >>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h >>>> index 11af6410dd52..7f3ed7e4b680 100644 >>>> --- a/include/linux/arm_sdei.h >>>> +++ b/include/linux/arm_sdei.h >>>> @@ -51,6 +51,7 @@ struct sdei_event { >>>> u32 event_num; >>>> u8 type; >>>> u8 priority; >>>> + u8 signaled; >>>> >>>> /* This pointer is handed to firmware as the event argument. */ >>>> union { >> Thanks, Gavin
On Mon, 27 Jul 2020 20:03:32 +1000 Gavin Shan <gshan@redhat.com> wrote: > Hi Jonathan, > > On 7/27/20 7:04 PM, Jonathan Cameron wrote: > > On Mon, 27 Jul 2020 10:53:27 +1000 > > Gavin Shan <gshan@redhat.com> wrote: > >> On 7/24/20 1:24 AM, Jonathan Cameron wrote: > >>> On Wed, 22 Jul 2020 19:57:39 +1000 > >>> Gavin Shan <gshan@redhat.com> wrote: > >>> > >>>> This retrieves the event signaled property when it's created for the > >>>> first time. The property will be needed when SDEI virtualization is > >>>> supported. > >>>> > >>>> Signed-off-by: Gavin Shan <gshan@redhat.com> > >>> > >>> These last two patches are probably fine but hard to tell without a user. > >>> > >> > >> Good question. Let me explain the background and please let me know > >> if you have more questions. SDEI was suggested by Marc to deliver > >> the notification during the asynchronous page fault, so that the > >> process can be rescheduled in guest. Unfortunately, we don't have > >> SDEI (or virtualized SDEI) supported yet. So the additional event > >> information is needed when SDEI virtualization is supported. > >> > >> The code of SDEI virtualization can be checked out from github: > >> > >> https://github.com/gwshan/linux/tree/sdei (branch: "sdei") > > Thanks. > > > > I'd be tempted to move these two patches to the next series > > that includes the users. > > > > I forgot to say, I'm fine with all the patches I didn't comment on. > > > > Yes, it's fine to move the last two patches to where we need > them. Thanks for your review and comments. May I have your > reviewed-by on those patches you didn't comment on? I would > like to pick the reviwed-by in v3 :) > Sure FWIW (I'm far from an expert in this area!) Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> for patches 1-7,13,15 as is 8-10 with trivial changes as discussed. 12 already given Given postponing 16 and 17, that just leaves 11 and 14 that I'd like to take a quick look at in v3. Jonathan > >> > >>>> --- > >>>> drivers/firmware/arm_sdei.c | 6 ++++++ > >>>> include/linux/arm_sdei.h | 1 + > >>>> 2 files changed, 7 insertions(+) > >>>> > >>>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c > >>>> index cf10fec57f2a..7518d3febf53 100644 > >>>> --- a/drivers/firmware/arm_sdei.c > >>>> +++ b/drivers/firmware/arm_sdei.c > >>>> @@ -204,6 +204,12 @@ static struct sdei_event *sdei_event_create(u32 event_num, > >>>> goto fail; > >>>> event->type = result; > >>>> > >>>> + err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_SIGNALED, > >>>> + &result); > >>>> + if (err) > >>>> + goto fail; > >>>> + event->signaled = result; > >>>> + > >>>> if (event->type == SDEI_EVENT_TYPE_SHARED) { > >>>> reg = kzalloc(sizeof(*reg), GFP_KERNEL); > >>>> if (!reg) { > >>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h > >>>> index 11af6410dd52..7f3ed7e4b680 100644 > >>>> --- a/include/linux/arm_sdei.h > >>>> +++ b/include/linux/arm_sdei.h > >>>> @@ -51,6 +51,7 @@ struct sdei_event { > >>>> u32 event_num; > >>>> u8 type; > >>>> u8 priority; > >>>> + u8 signaled; > >>>> > >>>> /* This pointer is handed to firmware as the event argument. */ > >>>> union { > >> > > Thanks, > Gavin >
Hi Jonathan, On 7/27/20 11:56 PM, Jonathan Cameron wrote: > On Mon, 27 Jul 2020 20:03:32 +1000 > Gavin Shan <gshan@redhat.com> wrote: >> On 7/27/20 7:04 PM, Jonathan Cameron wrote: >>> On Mon, 27 Jul 2020 10:53:27 +1000 >>> Gavin Shan <gshan@redhat.com> wrote: >>>> On 7/24/20 1:24 AM, Jonathan Cameron wrote: >>>>> On Wed, 22 Jul 2020 19:57:39 +1000 >>>>> Gavin Shan <gshan@redhat.com> wrote: >>>>> >>>>>> This retrieves the event signaled property when it's created for the >>>>>> first time. The property will be needed when SDEI virtualization is >>>>>> supported. >>>>>> >>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>>>> >>>>> These last two patches are probably fine but hard to tell without a user. >>>>> >>>> >>>> Good question. Let me explain the background and please let me know >>>> if you have more questions. SDEI was suggested by Marc to deliver >>>> the notification during the asynchronous page fault, so that the >>>> process can be rescheduled in guest. Unfortunately, we don't have >>>> SDEI (or virtualized SDEI) supported yet. So the additional event >>>> information is needed when SDEI virtualization is supported. >>>> >>>> The code of SDEI virtualization can be checked out from github: >>>> >>>> https://github.com/gwshan/linux/tree/sdei (branch: "sdei") >>> Thanks. >>> >>> I'd be tempted to move these two patches to the next series >>> that includes the users. >>> >>> I forgot to say, I'm fine with all the patches I didn't comment on. >>> >> >> Yes, it's fine to move the last two patches to where we need >> them. Thanks for your review and comments. May I have your >> reviewed-by on those patches you didn't comment on? I would >> like to pick the reviwed-by in v3 :) >> > Sure FWIW (I'm far from an expert in this area!) > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > for patches > 1-7,13,15 as is > 8-10 with trivial changes as discussed. > 12 already given > > Given postponing 16 and 17, that just leaves 11 and 14 that I'd > like to take a quick look at in v3. > Thanks for your confirmation. Except PATCH[11] and PATCH[14], all other patches in v3 will include your r-b. Thanks again for your time and comments :) >>>> >>>>>> --- >>>>>> drivers/firmware/arm_sdei.c | 6 ++++++ >>>>>> include/linux/arm_sdei.h | 1 + >>>>>> 2 files changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c >>>>>> index cf10fec57f2a..7518d3febf53 100644 >>>>>> --- a/drivers/firmware/arm_sdei.c >>>>>> +++ b/drivers/firmware/arm_sdei.c >>>>>> @@ -204,6 +204,12 @@ static struct sdei_event *sdei_event_create(u32 event_num, >>>>>> goto fail; >>>>>> event->type = result; >>>>>> >>>>>> + err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_SIGNALED, >>>>>> + &result); >>>>>> + if (err) >>>>>> + goto fail; >>>>>> + event->signaled = result; >>>>>> + >>>>>> if (event->type == SDEI_EVENT_TYPE_SHARED) { >>>>>> reg = kzalloc(sizeof(*reg), GFP_KERNEL); >>>>>> if (!reg) { >>>>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h >>>>>> index 11af6410dd52..7f3ed7e4b680 100644 >>>>>> --- a/include/linux/arm_sdei.h >>>>>> +++ b/include/linux/arm_sdei.h >>>>>> @@ -51,6 +51,7 @@ struct sdei_event { >>>>>> u32 event_num; >>>>>> u8 type; >>>>>> u8 priority; >>>>>> + u8 signaled; >>>>>> >>>>>> /* This pointer is handed to firmware as the event argument. */ >>>>>> union { >>>> Thanks, Gavin
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index cf10fec57f2a..7518d3febf53 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -204,6 +204,12 @@ static struct sdei_event *sdei_event_create(u32 event_num, goto fail; event->type = result; + err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_SIGNALED, + &result); + if (err) + goto fail; + event->signaled = result; + if (event->type == SDEI_EVENT_TYPE_SHARED) { reg = kzalloc(sizeof(*reg), GFP_KERNEL); if (!reg) { diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h index 11af6410dd52..7f3ed7e4b680 100644 --- a/include/linux/arm_sdei.h +++ b/include/linux/arm_sdei.h @@ -51,6 +51,7 @@ struct sdei_event { u32 event_num; u8 type; u8 priority; + u8 signaled; /* This pointer is handed to firmware as the event argument. */ union {
This retrieves the event signaled property when it's created for the first time. The property will be needed when SDEI virtualization is supported. Signed-off-by: Gavin Shan <gshan@redhat.com> --- drivers/firmware/arm_sdei.c | 6 ++++++ include/linux/arm_sdei.h | 1 + 2 files changed, 7 insertions(+)