| Message ID | 20240625185333.23211-2-admiyo@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MCTP over PCC | expand |
On Tue, Jun 25, 2024 at 02:53:31PM -0400, admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@amperecomputing.com> > > Type 4 PCC channels have an option to send back a response > to the platform when they are done processing the request. > The flag to indicate whether or not to respond is inside > the message body, and thus is not available to the pcc > mailbox. Since only one message can be processed at once per > channel, the value of this flag is checked during message processing > and passed back via the channels global structure. > > Ideally, the mailbox callback function would return a value > indicating whether the message requires an ACK, but that > would be a change to the mailbox API. That would involve > some change to all (about 12) of the mailbox based drivers, > and the majority of them would not need to know about the > ACK call. > Next time when you post new series, I prefer to be cc-ed in all the patches. So far I ignored v1 and v2 thinking it has landed in my mbox my mistake and deleted them. But just checked the series on lore, sorry for that. > Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> > --- > drivers/mailbox/pcc.c | 6 +++++- > include/acpi/pcc.h | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > index 94885e411085..5cf792700d79 100644 > --- a/drivers/mailbox/pcc.c > +++ b/drivers/mailbox/pcc.c > @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > { > struct pcc_chan_info *pchan; > struct mbox_chan *chan = p; > + struct pcc_mbox_chan *pmchan; > u64 val; > int ret; > > @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) > return IRQ_NONE; > > + pmchan = &pchan->chan; > + pmchan->ack_rx = true; //TODO default to False Indeed, default must be false. You need to do this conditionally at runtime otherwise I see no need for this patch as it doesn't change anything as it stands. It needs to be fixed to get this change merged. Also we should set any such flag once at the boot, IRQ handler is not the right place for sure.
On 6/26/24 08:27, Sudeep Holla wrote: > On Tue, Jun 25, 2024 at 02:53:31PM -0400, admiyo@os.amperecomputing.com wrote: >> From: Adam Young <admiyo@amperecomputing.com> >> >> Type 4 PCC channels have an option to send back a response >> to the platform when they are done processing the request. >> The flag to indicate whether or not to respond is inside >> the message body, and thus is not available to the pcc >> mailbox. Since only one message can be processed at once per >> channel, the value of this flag is checked during message processing >> and passed back via the channels global structure. >> >> Ideally, the mailbox callback function would return a value >> indicating whether the message requires an ACK, but that >> would be a change to the mailbox API. That would involve >> some change to all (about 12) of the mailbox based drivers, >> and the majority of them would not need to know about the >> ACK call. >> > Next time when you post new series, I prefer to be cc-ed in all the patches. I was using the list of maintainers for each patch as pulled via the Kernel scripts in git send-email for the first two versions. I have started hard coding the list of CCers as a superset of all the maintainers, as this patch series crosses a few boundaries. > So far I ignored v1 and v2 thinking it has landed in my mbox my mistake and > deleted them. But just checked the series on lore, sorry for that. > >> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> >> --- >> drivers/mailbox/pcc.c | 6 +++++- >> include/acpi/pcc.h | 1 + >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c >> index 94885e411085..5cf792700d79 100644 >> --- a/drivers/mailbox/pcc.c >> +++ b/drivers/mailbox/pcc.c >> @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) >> { >> struct pcc_chan_info *pchan; >> struct mbox_chan *chan = p; >> + struct pcc_mbox_chan *pmchan; >> u64 val; >> int ret; >> >> @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) >> if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) >> return IRQ_NONE; >> >> + pmchan = &pchan->chan; >> + pmchan->ack_rx = true; //TODO default to False > Indeed, default must be false. You need to do this conditionally at runtime > otherwise I see no need for this patch as it doesn't change anything as it > stands. It needs to be fixed to get this change merged. > > Also we should set any such flag once at the boot, IRQ handler is not > the right place for sure. Ringing the doorbell to signify that the message is received is optional in the ACPI/PCC spec but was hard coded to always get set. There is currently no way to pass the value from the rx function to here where the code is actually triggering the response. The Mailbox receive callback returns void. Changing that would require changing every module that uses the mailbox code. You can see the spec here: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#platform-notification-for-slave-pcc-subspaces-type-4 note in step 2 of the send process, the part that says "The platform can request the OSPM rings the doorbell once it has completed processing the notification command by setting the Generate Signal bit in the flags." The change that made the response mandatory was 60c40b06fa686 and merged in the 6.9 timeframe. The actual check is done at run time, and you can see how it is done in the third patch, at the end of the function mctp_pcc_client_rx_callback + flags = mctp_pcc_hdr.flags; + mctp_pcc_dev->in_chan->ack_rx = (flags & PCC_ACK_FLAG_MASK) > 0; While we don't anticipate our backend requiring the ACK, we did encode the possibility into the driver. I am willing to rework this if we have a viable alternative. >
Huisong Li you wrote the original patch that I am working around. What would I break if I disabled the IRQ ACK? It should not be the default behavior as it is an optional feature. There needs to be a mechanism for the driver to trigger the ACK, but it needs to be based on the content of the message buffer. I was thinking it should be in the return code of the callback, but that breaks all of the other mailbox implementations. I suspect a better approach would be to provide a function pointer to the driver and let the driver decide whether or not to trigger the ACK. On 6/26/24 08:27, Sudeep Holla wrote: > On Tue, Jun 25, 2024 at 02:53:31PM -0400, admiyo@os.amperecomputing.com wrote: >> From: Adam Young <admiyo@amperecomputing.com> >> >> Type 4 PCC channels have an option to send back a response >> to the platform when they are done processing the request. >> The flag to indicate whether or not to respond is inside >> the message body, and thus is not available to the pcc >> mailbox. Since only one message can be processed at once per >> channel, the value of this flag is checked during message processing >> and passed back via the channels global structure. >> >> Ideally, the mailbox callback function would return a value >> indicating whether the message requires an ACK, but that >> would be a change to the mailbox API. That would involve >> some change to all (about 12) of the mailbox based drivers, >> and the majority of them would not need to know about the >> ACK call. >> > Next time when you post new series, I prefer to be cc-ed in all the patches. > So far I ignored v1 and v2 thinking it has landed in my mbox my mistake and > deleted them. But just checked the series on lore, sorry for that. > >> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> >> --- >> drivers/mailbox/pcc.c | 6 +++++- >> include/acpi/pcc.h | 1 + >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c >> index 94885e411085..5cf792700d79 100644 >> --- a/drivers/mailbox/pcc.c >> +++ b/drivers/mailbox/pcc.c >> @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) >> { >> struct pcc_chan_info *pchan; >> struct mbox_chan *chan = p; >> + struct pcc_mbox_chan *pmchan; >> u64 val; >> int ret; >> >> @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) >> if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) >> return IRQ_NONE; >> >> + pmchan = &pchan->chan; >> + pmchan->ack_rx = true; //TODO default to False > Indeed, default must be false. You need to do this conditionally at runtime > otherwise I see no need for this patch as it doesn't change anything as it > stands. It needs to be fixed to get this change merged. > > Also we should set any such flag once at the boot, IRQ handler is not > the right place for sure. >
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index 94885e411085..5cf792700d79 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) { struct pcc_chan_info *pchan; struct mbox_chan *chan = p; + struct pcc_mbox_chan *pmchan; u64 val; int ret; @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) return IRQ_NONE; + pmchan = &pchan->chan; + pmchan->ack_rx = true; //TODO default to False mbox_chan_received_data(chan, NULL); /* @@ -312,7 +315,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) * * The PCC master subspace channel clears chan_in_use to free channel. */ - if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE && + pmchan->ack_rx) pcc_send_data(chan, NULL); pchan->chan_in_use = false; diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h index 9b373d172a77..297913378c2b 100644 --- a/include/acpi/pcc.h +++ b/include/acpi/pcc.h @@ -16,6 +16,7 @@ struct pcc_mbox_chan { u32 latency; u32 max_access_rate; u16 min_turnaround_time; + bool ack_rx; }; /* Generic Communications Channel Shared Memory Region */