* [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency @ 2022-08-12 14:47 Andrew Jeffery 2022-10-05 23:12 ` Andrew Jeffery 2022-10-05 23:50 ` Joel Stanley 0 siblings, 2 replies; 7+ messages in thread From: Andrew Jeffery @ 2022-08-12 14:47 UTC (permalink / raw) To: openipmi-developer Cc: minyard, joel, linux-arm-kernel, linux-aspeed, linux-kernel, openbmc The ASPEED KCS devices don't provide a BMC-side interrupt for the host reading the output data register (ODR). The act of the host reading ODR clears the output buffer full (OBF) flag in the status register (STR), informing the BMC it can transmit a subsequent byte. On the BMC side the KCS client must enable the OBE event *and* perform a subsequent read of STR anyway to avoid races - the polling provides a window for the host to read ODR if data was freshly written while minimising BMC-side latency. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/char/ipmi/kcs_bmc_aspeed.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c index cdc88cde1e9a..417e5a3ccfae 100644 --- a/drivers/char/ipmi/kcs_bmc_aspeed.c +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c @@ -399,13 +399,31 @@ static void aspeed_kcs_check_obe(struct timer_list *timer) static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 state) { struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc); + int rc; + u8 str; /* We don't have an OBE IRQ, emulate it */ if (mask & KCS_BMC_EVENT_TYPE_OBE) { - if (KCS_BMC_EVENT_TYPE_OBE & state) - mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); - else + if (KCS_BMC_EVENT_TYPE_OBE & state) { + /* + * Given we don't have an OBE IRQ, delay by polling briefly to see if we can + * observe such an event before returning to the caller. This is not + * incorrect because OBF may have already become clear before enabling the + * IRQ if we had one, under which circumstance no event will be propagated + * anyway. + * + * The onus is on the client to perform a race-free check that it hasn't + * missed the event. + */ + rc = read_poll_timeout_atomic(aspeed_kcs_inb, str, + !(str & KCS_BMC_STR_OBF), 1, 100, false, + &priv->kcs_bmc, priv->kcs_bmc.ioreg.str); + /* Time for the slow path? */ + if (rc == -ETIMEDOUT) + mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); + } else { del_timer(&priv->obe.timer); + } } if (mask & KCS_BMC_EVENT_TYPE_IBF) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency 2022-08-12 14:47 [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency Andrew Jeffery @ 2022-10-05 23:12 ` Andrew Jeffery 2022-10-05 23:54 ` Corey Minyard 2022-10-05 23:50 ` Joel Stanley 1 sibling, 1 reply; 7+ messages in thread From: Andrew Jeffery @ 2022-10-05 23:12 UTC (permalink / raw) To: openipmi-developer, Corey Minyard Cc: linux-aspeed, openbmc, linux-kernel, linux-arm-kernel Hi Corey, On Sat, 13 Aug 2022, at 00:17, Andrew Jeffery wrote: > The ASPEED KCS devices don't provide a BMC-side interrupt for the host > reading the output data register (ODR). The act of the host reading ODR > clears the output buffer full (OBF) flag in the status register (STR), > informing the BMC it can transmit a subsequent byte. > > On the BMC side the KCS client must enable the OBE event *and* perform a > subsequent read of STR anyway to avoid races - the polling provides a > window for the host to read ODR if data was freshly written while > minimising BMC-side latency. Just wondering whether you're happy to pick this one up? I haven't seen it hit the IPMI tree yet. Cheers, Andrew ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency 2022-10-05 23:12 ` Andrew Jeffery @ 2022-10-05 23:54 ` Corey Minyard 2022-10-06 3:08 ` Andrew Jeffery 0 siblings, 1 reply; 7+ messages in thread From: Corey Minyard @ 2022-10-05 23:54 UTC (permalink / raw) To: Andrew Jeffery Cc: openipmi-developer, linux-aspeed, openbmc, linux-kernel, linux-arm-kernel On Thu, Oct 06, 2022 at 09:42:57AM +1030, Andrew Jeffery wrote: > Hi Corey, > > On Sat, 13 Aug 2022, at 00:17, Andrew Jeffery wrote: > > The ASPEED KCS devices don't provide a BMC-side interrupt for the host > > reading the output data register (ODR). The act of the host reading ODR > > clears the output buffer full (OBF) flag in the status register (STR), > > informing the BMC it can transmit a subsequent byte. > > > > On the BMC side the KCS client must enable the OBE event *and* perform a > > subsequent read of STR anyway to avoid races - the polling provides a > > window for the host to read ODR if data was freshly written while > > minimising BMC-side latency. > > Just wondering whether you're happy to pick this one up? I haven't seen > it hit the IPMI tree yet. Sorry. It's in my tree for 6.2 right now. I can't push it up to for-next until 6.1-rc1 comes out. -corey > > Cheers, > > Andrew ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency 2022-10-05 23:54 ` Corey Minyard @ 2022-10-06 3:08 ` Andrew Jeffery 0 siblings, 0 replies; 7+ messages in thread From: Andrew Jeffery @ 2022-10-06 3:08 UTC (permalink / raw) To: Corey Minyard Cc: openipmi-developer, linux-aspeed, openbmc, linux-kernel, linux-arm-kernel On Thu, 6 Oct 2022, at 10:24, Corey Minyard wrote: > On Thu, Oct 06, 2022 at 09:42:57AM +1030, Andrew Jeffery wrote: >> Hi Corey, >> >> On Sat, 13 Aug 2022, at 00:17, Andrew Jeffery wrote: >> > The ASPEED KCS devices don't provide a BMC-side interrupt for the host >> > reading the output data register (ODR). The act of the host reading ODR >> > clears the output buffer full (OBF) flag in the status register (STR), >> > informing the BMC it can transmit a subsequent byte. >> > >> > On the BMC side the KCS client must enable the OBE event *and* perform a >> > subsequent read of STR anyway to avoid races - the polling provides a >> > window for the host to read ODR if data was freshly written while >> > minimising BMC-side latency. >> >> Just wondering whether you're happy to pick this one up? I haven't seen >> it hit the IPMI tree yet. > > Sorry. It's in my tree for 6.2 right now. Thanks! > > I can't push it up to for-next until 6.1-rc1 comes out. > No worries, just wanted to make sure it didn't fall through the cracks :) Andrew ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency 2022-08-12 14:47 [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency Andrew Jeffery 2022-10-05 23:12 ` Andrew Jeffery @ 2022-10-05 23:50 ` Joel Stanley 2022-10-06 3:06 ` Andrew Jeffery 1 sibling, 1 reply; 7+ messages in thread From: Joel Stanley @ 2022-10-05 23:50 UTC (permalink / raw) To: Andrew Jeffery Cc: openipmi-developer, minyard, linux-arm-kernel, linux-aspeed, linux-kernel, openbmc On Fri, 12 Aug 2022 at 14:48, Andrew Jeffery <andrew@aj.id.au> wrote: > > The ASPEED KCS devices don't provide a BMC-side interrupt for the host > reading the output data register (ODR). The act of the host reading ODR > clears the output buffer full (OBF) flag in the status register (STR), > informing the BMC it can transmit a subsequent byte. > > On the BMC side the KCS client must enable the OBE event *and* perform a > subsequent read of STR anyway to avoid races - the polling provides a > window for the host to read ODR if data was freshly written while > minimising BMC-side latency. > Fixes...? > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > drivers/char/ipmi/kcs_bmc_aspeed.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c > index cdc88cde1e9a..417e5a3ccfae 100644 > --- a/drivers/char/ipmi/kcs_bmc_aspeed.c > +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c > @@ -399,13 +399,31 @@ static void aspeed_kcs_check_obe(struct timer_list *timer) > static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 state) > { > struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc); > + int rc; > + u8 str; str is status, it would be good to spell that out in full. > > /* We don't have an OBE IRQ, emulate it */ > if (mask & KCS_BMC_EVENT_TYPE_OBE) { > - if (KCS_BMC_EVENT_TYPE_OBE & state) > - mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); > - else > + if (KCS_BMC_EVENT_TYPE_OBE & state) { > + /* > + * Given we don't have an OBE IRQ, delay by polling briefly to see if we can > + * observe such an event before returning to the caller. This is not > + * incorrect because OBF may have already become clear before enabling the > + * IRQ if we had one, under which circumstance no event will be propagated > + * anyway. > + * > + * The onus is on the client to perform a race-free check that it hasn't > + * missed the event. > + */ > + rc = read_poll_timeout_atomic(aspeed_kcs_inb, str, > + !(str & KCS_BMC_STR_OBF), 1, 100, false, > + &priv->kcs_bmc, priv->kcs_bmc.ioreg.str); > + /* Time for the slow path? */ The mod_timer is the slow path? The question mark threw me. > + if (rc == -ETIMEDOUT) > + mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); > + } else { > del_timer(&priv->obe.timer); > + } > } > > if (mask & KCS_BMC_EVENT_TYPE_IBF) { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency 2022-10-05 23:50 ` Joel Stanley @ 2022-10-06 3:06 ` Andrew Jeffery 2022-10-06 14:09 ` Corey Minyard 0 siblings, 1 reply; 7+ messages in thread From: Andrew Jeffery @ 2022-10-06 3:06 UTC (permalink / raw) To: Joel Stanley Cc: openipmi-developer, Corey Minyard, linux-arm-kernel, linux-aspeed, linux-kernel, openbmc On Thu, 6 Oct 2022, at 10:20, Joel Stanley wrote: > On Fri, 12 Aug 2022 at 14:48, Andrew Jeffery <andrew@aj.id.au> wrote: >> >> The ASPEED KCS devices don't provide a BMC-side interrupt for the host >> reading the output data register (ODR). The act of the host reading ODR >> clears the output buffer full (OBF) flag in the status register (STR), >> informing the BMC it can transmit a subsequent byte. >> >> On the BMC side the KCS client must enable the OBE event *and* perform a >> subsequent read of STR anyway to avoid races - the polling provides a >> window for the host to read ODR if data was freshly written while >> minimising BMC-side latency. >> > > Fixes...? Is it a fix though? It's definitely an *improvement* in behaviour, but the existing behaviour also wasn't *incorrect*, just kinda unfortunate under certain timings? Dunno. I'm probably splitting hairs. In any case, if we do want a fixes line: Fixes: 28651e6c4237 ("ipmi: kcs_bmc: Allow clients to control KCS IRQ state") > >> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > Reviewed-by: Joel Stanley <joel@jms.id.au> Thanks! > >> --- >> drivers/char/ipmi/kcs_bmc_aspeed.c | 24 +++++++++++++++++++++--- >> 1 file changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c >> index cdc88cde1e9a..417e5a3ccfae 100644 >> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c >> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c >> @@ -399,13 +399,31 @@ static void aspeed_kcs_check_obe(struct timer_list *timer) >> static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 state) >> { >> struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc); >> + int rc; >> + u8 str; > > str is status, it would be good to spell that out in full. I guess if it trips enough people up we can rename it later. > >> >> /* We don't have an OBE IRQ, emulate it */ >> if (mask & KCS_BMC_EVENT_TYPE_OBE) { >> - if (KCS_BMC_EVENT_TYPE_OBE & state) >> - mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); >> - else >> + if (KCS_BMC_EVENT_TYPE_OBE & state) { >> + /* >> + * Given we don't have an OBE IRQ, delay by polling briefly to see if we can >> + * observe such an event before returning to the caller. This is not >> + * incorrect because OBF may have already become clear before enabling the >> + * IRQ if we had one, under which circumstance no event will be propagated >> + * anyway. >> + * >> + * The onus is on the client to perform a race-free check that it hasn't >> + * missed the event. >> + */ >> + rc = read_poll_timeout_atomic(aspeed_kcs_inb, str, >> + !(str & KCS_BMC_STR_OBF), 1, 100, false, >> + &priv->kcs_bmc, priv->kcs_bmc.ioreg.str); >> + /* Time for the slow path? */ > > The mod_timer is the slow path? The question mark threw me. Yeah, mod_timer() is the slow path; read_poll_timeout_atomic() is the fast path and we've exhausted the time we're willing to wait there if we get -ETIMEDOUT. The comment was intended as a description for the question posed by the condition. It made sense in my head but maybe it's confusing more than it is enlightening? Andrew > >> + if (rc == -ETIMEDOUT) >> + mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); >> + } else { >> del_timer(&priv->obe.timer); >> + } >> } >> >> if (mask & KCS_BMC_EVENT_TYPE_IBF) { >> -- >> 2.34.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency 2022-10-06 3:06 ` Andrew Jeffery @ 2022-10-06 14:09 ` Corey Minyard 0 siblings, 0 replies; 7+ messages in thread From: Corey Minyard @ 2022-10-06 14:09 UTC (permalink / raw) To: Andrew Jeffery Cc: Joel Stanley, openipmi-developer, linux-arm-kernel, linux-aspeed, linux-kernel, openbmc On Thu, Oct 06, 2022 at 01:36:51PM +1030, Andrew Jeffery wrote: > > > On Thu, 6 Oct 2022, at 10:20, Joel Stanley wrote: > > On Fri, 12 Aug 2022 at 14:48, Andrew Jeffery <andrew@aj.id.au> wrote: > >> > >> The ASPEED KCS devices don't provide a BMC-side interrupt for the host > >> reading the output data register (ODR). The act of the host reading ODR > >> clears the output buffer full (OBF) flag in the status register (STR), > >> informing the BMC it can transmit a subsequent byte. > >> > >> On the BMC side the KCS client must enable the OBE event *and* perform a > >> subsequent read of STR anyway to avoid races - the polling provides a > >> window for the host to read ODR if data was freshly written while > >> minimising BMC-side latency. > >> > > > > Fixes...? > > Is it a fix though? It's definitely an *improvement* in behaviour, but > the existing behaviour also wasn't *incorrect*, just kinda unfortunate > under certain timings? Dunno. I'm probably splitting hairs. > > In any case, if we do want a fixes line: > > Fixes: 28651e6c4237 ("ipmi: kcs_bmc: Allow clients to control KCS IRQ state") I added the Fixes and Joel's review. Thanks, -corey > > > > >> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > Thanks! > > > > >> --- > >> drivers/char/ipmi/kcs_bmc_aspeed.c | 24 +++++++++++++++++++++--- > >> 1 file changed, 21 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c > >> index cdc88cde1e9a..417e5a3ccfae 100644 > >> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c > >> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c > >> @@ -399,13 +399,31 @@ static void aspeed_kcs_check_obe(struct timer_list *timer) > >> static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 state) > >> { > >> struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc); > >> + int rc; > >> + u8 str; > > > > str is status, it would be good to spell that out in full. > > I guess if it trips enough people up we can rename it later. > > > > >> > >> /* We don't have an OBE IRQ, emulate it */ > >> if (mask & KCS_BMC_EVENT_TYPE_OBE) { > >> - if (KCS_BMC_EVENT_TYPE_OBE & state) > >> - mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); > >> - else > >> + if (KCS_BMC_EVENT_TYPE_OBE & state) { > >> + /* > >> + * Given we don't have an OBE IRQ, delay by polling briefly to see if we can > >> + * observe such an event before returning to the caller. This is not > >> + * incorrect because OBF may have already become clear before enabling the > >> + * IRQ if we had one, under which circumstance no event will be propagated > >> + * anyway. > >> + * > >> + * The onus is on the client to perform a race-free check that it hasn't > >> + * missed the event. > >> + */ > >> + rc = read_poll_timeout_atomic(aspeed_kcs_inb, str, > >> + !(str & KCS_BMC_STR_OBF), 1, 100, false, > >> + &priv->kcs_bmc, priv->kcs_bmc.ioreg.str); > >> + /* Time for the slow path? */ > > > > The mod_timer is the slow path? The question mark threw me. > > Yeah, mod_timer() is the slow path; read_poll_timeout_atomic() is the > fast path and we've exhausted the time we're willing to wait there if > we get -ETIMEDOUT. > > The comment was intended as a description for the question posed by the > condition. It made sense in my head but maybe it's confusing more than > it is enlightening? > > Andrew > > > > >> + if (rc == -ETIMEDOUT) > >> + mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); > >> + } else { > >> del_timer(&priv->obe.timer); > >> + } > >> } > >> > >> if (mask & KCS_BMC_EVENT_TYPE_IBF) { > >> -- > >> 2.34.1 > >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-06 14:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-12 14:47 [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency Andrew Jeffery 2022-10-05 23:12 ` Andrew Jeffery 2022-10-05 23:54 ` Corey Minyard 2022-10-06 3:08 ` Andrew Jeffery 2022-10-05 23:50 ` Joel Stanley 2022-10-06 3:06 ` Andrew Jeffery 2022-10-06 14:09 ` Corey Minyard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).