| Message ID | 20181112151853.29289-2-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: Add Tegra Combined UART driver | expand |
On Mon, Nov 12, 2018 at 9:18 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > From: Thierry Reding <treding@nvidia.com> > > The mailbox framework supports blocking transfers via completions for > clients that can sleep. In order to support blocking transfers in cases > where the transmission is not permitted to sleep, add a new ->flush() > callback that controller drivers can implement to busy loop until the > transmission has been completed. This will automatically be called when > available and interrupts are disabled for clients that request blocking > transfers. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/mailbox/mailbox.c | 8 ++++++++ > include/linux/mailbox_controller.h | 4 ++++ > 2 files changed, 12 insertions(+) > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > index 674b35f402f5..0eaf21259874 100644 > --- a/drivers/mailbox/mailbox.c > +++ b/drivers/mailbox/mailbox.c > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > unsigned long wait; > int ret; > > + if (irqs_disabled() && chan->mbox->ops->flush) { > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > + if (ret < 0) > + tx_tick(chan, ret); > + > + return ret; > + } > + This is hacky. I think we can do without busy waiting in atomic context. You could queue locally while in atomic context and then transfer in blocking mode. I don't think we should worry about the 'throughput' as there already is no h/w rate control even with busy-waiting. Thanks.
On Sat, Nov 17, 2018 at 11:27:17AM -0600, Jassi Brar wrote: > On Mon, Nov 12, 2018 at 9:18 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > The mailbox framework supports blocking transfers via completions for > > clients that can sleep. In order to support blocking transfers in cases > > where the transmission is not permitted to sleep, add a new ->flush() > > callback that controller drivers can implement to busy loop until the > > transmission has been completed. This will automatically be called when > > available and interrupts are disabled for clients that request blocking > > transfers. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/mailbox/mailbox.c | 8 ++++++++ > > include/linux/mailbox_controller.h | 4 ++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > index 674b35f402f5..0eaf21259874 100644 > > --- a/drivers/mailbox/mailbox.c > > +++ b/drivers/mailbox/mailbox.c > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > unsigned long wait; > > int ret; > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > + if (ret < 0) > > + tx_tick(chan, ret); > > + > > + return ret; > > + } > > + > This is hacky. I think we can do without busy waiting in atomic > context. You could queue locally while in atomic context and then > transfer in blocking mode. I don't think we should worry about the > 'throughput' as there already is no h/w rate control even with > busy-waiting. I actually tried to do that before I added this flushing mechanism. The problem is, like you said, one of rate control. As mentioned in the cover letter, the shared mailboxes implemented in tegra-hsp are used as RX and TX channels for the TCU, which is like a virtual UART. The TTY driver included as part of this series will use one of the mailboxes to transmit data that is written to the console. The problem is that if these transmissions are not rate-limited on the TTY driver side, the console will just keep writing data and eventually overflow the buffer that we have in the mailbox subsystem. The problem is that data comes in at a much higher rate than what we can output. This is especially true at boot when the TCU console takes over and the whole log buffer is dumped on it. So the only way to rate-limit is to either make mbox_send_message() block, but that can only be done in non-atomic context. The console, however, will always run in atomic context, so the only way to do rate- limiting is by busy looping. Thierry
On Tue, Nov 20, 2018 at 04:29:07PM +0100, Thierry Reding wrote: > On Sat, Nov 17, 2018 at 11:27:17AM -0600, Jassi Brar wrote: > > On Mon, Nov 12, 2018 at 9:18 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > The mailbox framework supports blocking transfers via completions for > > > clients that can sleep. In order to support blocking transfers in cases > > > where the transmission is not permitted to sleep, add a new ->flush() > > > callback that controller drivers can implement to busy loop until the > > > transmission has been completed. This will automatically be called when > > > available and interrupts are disabled for clients that request blocking > > > transfers. > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > include/linux/mailbox_controller.h | 4 ++++ > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > index 674b35f402f5..0eaf21259874 100644 > > > --- a/drivers/mailbox/mailbox.c > > > +++ b/drivers/mailbox/mailbox.c > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > unsigned long wait; > > > int ret; > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > + if (ret < 0) > > > + tx_tick(chan, ret); > > > + > > > + return ret; > > > + } > > > + > > This is hacky. I think we can do without busy waiting in atomic > > context. You could queue locally while in atomic context and then > > transfer in blocking mode. I don't think we should worry about the > > 'throughput' as there already is no h/w rate control even with > > busy-waiting. > > I actually tried to do that before I added this flushing mechanism. The > problem is, like you said, one of rate control. As mentioned in the > cover letter, the shared mailboxes implemented in tegra-hsp are used as > RX and TX channels for the TCU, which is like a virtual UART. The TTY > driver included as part of this series will use one of the mailboxes to > transmit data that is written to the console. The problem is that if > these transmissions are not rate-limited on the TTY driver side, the > console will just keep writing data and eventually overflow the buffer > that we have in the mailbox subsystem. > > The problem is that data comes in at a much higher rate than what we can > output. This is especially true at boot when the TCU console takes over > and the whole log buffer is dumped on it. > > So the only way to rate-limit is to either make mbox_send_message() > block, but that can only be done in non-atomic context. The console, > however, will always run in atomic context, so the only way to do rate- > limiting is by busy looping. What I also tried before was to implement busy looping within the ->send_data() callback of the driver so that we didn't have to put this into the core. Unfortunately, however, the ->send_data() callback is called under chan->lock, which means that from mbox_send_message() we don't have a way to mark the transfer as done. In order to do that we'd have to call mbox_chan_txdone(), but that ends up calling tx_tick() and that in turn also attempts to take the chan->lock, which would cause a deadlock. The explicit flushing is the best alternative that I could come up with. I think it's not all that hacky, because it's very explicit about what's going on and it has the nice side-effect that it will allow the mailbox to work in interrupt driven mode if possible and only resorting to the busy loop in atomic context. At this point I think I have explored all other options and I frankly can't find a more proper way to achieve what we need here. Perhaps you can think of additional ways to accomplish this? Thierry
On Wed, Nov 21, 2018 at 8:27 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Tue, Nov 20, 2018 at 04:29:07PM +0100, Thierry Reding wrote: > > On Sat, Nov 17, 2018 at 11:27:17AM -0600, Jassi Brar wrote: > > > On Mon, Nov 12, 2018 at 9:18 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > The mailbox framework supports blocking transfers via completions for > > > > clients that can sleep. In order to support blocking transfers in cases > > > > where the transmission is not permitted to sleep, add a new ->flush() > > > > callback that controller drivers can implement to busy loop until the > > > > transmission has been completed. This will automatically be called when > > > > available and interrupts are disabled for clients that request blocking > > > > transfers. > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > --- > > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > > include/linux/mailbox_controller.h | 4 ++++ > > > > 2 files changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > > index 674b35f402f5..0eaf21259874 100644 > > > > --- a/drivers/mailbox/mailbox.c > > > > +++ b/drivers/mailbox/mailbox.c > > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > > unsigned long wait; > > > > int ret; > > > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > > + if (ret < 0) > > > > + tx_tick(chan, ret); > > > > + > > > > + return ret; > > > > + } > > > > + > > > This is hacky. I think we can do without busy waiting in atomic > > > context. You could queue locally while in atomic context and then > > > transfer in blocking mode. I don't think we should worry about the > > > 'throughput' as there already is no h/w rate control even with > > > busy-waiting. > > > > I actually tried to do that before I added this flushing mechanism. The > > problem is, like you said, one of rate control. As mentioned in the > > cover letter, the shared mailboxes implemented in tegra-hsp are used as > > RX and TX channels for the TCU, which is like a virtual UART. The TTY > > driver included as part of this series will use one of the mailboxes to > > transmit data that is written to the console. The problem is that if > > these transmissions are not rate-limited on the TTY driver side, the > > console will just keep writing data and eventually overflow the buffer > > that we have in the mailbox subsystem. > > > > The problem is that data comes in at a much higher rate than what we can > > output. This is especially true at boot when the TCU console takes over > > and the whole log buffer is dumped on it. > > > > So the only way to rate-limit is to either make mbox_send_message() > > block, but that can only be done in non-atomic context. The console, > > however, will always run in atomic context, so the only way to do rate- > > limiting is by busy looping. > > What I also tried before was to implement busy looping within the > ->send_data() callback of the driver so that we didn't have to put this > into the core. Unfortunately, however, the ->send_data() callback is > called under chan->lock, which means that from mbox_send_message() we > don't have a way to mark the transfer as done. In order to do that we'd > have to call mbox_chan_txdone(), but that ends up calling tx_tick() and > that in turn also attempts to take the chan->lock, which would cause a > deadlock. > > The explicit flushing is the best alternative that I could come up with. > I think it's not all that hacky, because it's very explicit about what's > going on and it has the nice side-effect that it will allow the mailbox > to work in interrupt driven mode if possible and only resorting to the > busy loop in atomic context. > > At this point I think I have explored all other options and I frankly > can't find a more proper way to achieve what we need here. Perhaps you > can think of additional ways to accomplish this? > Well, I would have a local ring buffer (array) of enough size to hold the characters and then have a task consuming data from that ring buffer by transmitting over mailbox. -jassi
On Wed, Nov 21, 2018 at 08:18:22PM -0600, Jassi Brar wrote: > On Wed, Nov 21, 2018 at 8:27 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Tue, Nov 20, 2018 at 04:29:07PM +0100, Thierry Reding wrote: > > > On Sat, Nov 17, 2018 at 11:27:17AM -0600, Jassi Brar wrote: > > > > On Mon, Nov 12, 2018 at 9:18 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > > > The mailbox framework supports blocking transfers via completions for > > > > > clients that can sleep. In order to support blocking transfers in cases > > > > > where the transmission is not permitted to sleep, add a new ->flush() > > > > > callback that controller drivers can implement to busy loop until the > > > > > transmission has been completed. This will automatically be called when > > > > > available and interrupts are disabled for clients that request blocking > > > > > transfers. > > > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > > --- > > > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > > > include/linux/mailbox_controller.h | 4 ++++ > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > > > index 674b35f402f5..0eaf21259874 100644 > > > > > --- a/drivers/mailbox/mailbox.c > > > > > +++ b/drivers/mailbox/mailbox.c > > > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > > > unsigned long wait; > > > > > int ret; > > > > > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > > > + if (ret < 0) > > > > > + tx_tick(chan, ret); > > > > > + > > > > > + return ret; > > > > > + } > > > > > + > > > > This is hacky. I think we can do without busy waiting in atomic > > > > context. You could queue locally while in atomic context and then > > > > transfer in blocking mode. I don't think we should worry about the > > > > 'throughput' as there already is no h/w rate control even with > > > > busy-waiting. > > > > > > I actually tried to do that before I added this flushing mechanism. The > > > problem is, like you said, one of rate control. As mentioned in the > > > cover letter, the shared mailboxes implemented in tegra-hsp are used as > > > RX and TX channels for the TCU, which is like a virtual UART. The TTY > > > driver included as part of this series will use one of the mailboxes to > > > transmit data that is written to the console. The problem is that if > > > these transmissions are not rate-limited on the TTY driver side, the > > > console will just keep writing data and eventually overflow the buffer > > > that we have in the mailbox subsystem. > > > > > > The problem is that data comes in at a much higher rate than what we can > > > output. This is especially true at boot when the TCU console takes over > > > and the whole log buffer is dumped on it. > > > > > > So the only way to rate-limit is to either make mbox_send_message() > > > block, but that can only be done in non-atomic context. The console, > > > however, will always run in atomic context, so the only way to do rate- > > > limiting is by busy looping. > > > > What I also tried before was to implement busy looping within the > > ->send_data() callback of the driver so that we didn't have to put this > > into the core. Unfortunately, however, the ->send_data() callback is > > called under chan->lock, which means that from mbox_send_message() we > > don't have a way to mark the transfer as done. In order to do that we'd > > have to call mbox_chan_txdone(), but that ends up calling tx_tick() and > > that in turn also attempts to take the chan->lock, which would cause a > > deadlock. > > > > The explicit flushing is the best alternative that I could come up with. > > I think it's not all that hacky, because it's very explicit about what's > > going on and it has the nice side-effect that it will allow the mailbox > > to work in interrupt driven mode if possible and only resorting to the > > busy loop in atomic context. > > > > At this point I think I have explored all other options and I frankly > > can't find a more proper way to achieve what we need here. Perhaps you > > can think of additional ways to accomplish this? > > > Well, I would have a local ring buffer (array) of enough size to hold > the characters and then have a task consuming data from that ring > buffer by transmitting over mailbox. There's already such a ringbuffer in the printk code. To implement what you suggest would effectively be creating a copy of that buffer because we'd be allocating the buffer and the console code would just dump each and every character in the logbuf into that ring buffer without rate- limitation. To make matters worse, the ringbuffer would be empty most of the time after the initial dump of the logbuf, so we'd be wasting all that buffer space. It just seems to me like we should be keeping the TCU driver as close as possible to other UART drivers which also busy loop in order to rate- limit what the console can write. Given the current mailbox framework it is not possible to do that (in interrupt context), so an extension seems like the most sensible option. Perhaps you'd be less concerned about such a change if it was perhaps more explicit? Just throwing ideas around, I think something that could also work is if we explicitly add a mbox_flush() function that would basically be calling ->flush(). That way users of the mailbox can make their requirement very explicit. I haven't actually tested that, but I think it would work. Does that sound more acceptable to you? Thierry
Hi Thierry, On Thu, Nov 22, 2018 at 2:47 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Nov 21, 2018 at 08:18:22PM -0600, Jassi Brar wrote: > > On Wed, Nov 21, 2018 at 8:27 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Tue, Nov 20, 2018 at 04:29:07PM +0100, Thierry Reding wrote: > > > > On Sat, Nov 17, 2018 at 11:27:17AM -0600, Jassi Brar wrote: > > > > > On Mon, Nov 12, 2018 at 9:18 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > > > > > The mailbox framework supports blocking transfers via completions for > > > > > > clients that can sleep. In order to support blocking transfers in cases > > > > > > where the transmission is not permitted to sleep, add a new ->flush() > > > > > > callback that controller drivers can implement to busy loop until the > > > > > > transmission has been completed. This will automatically be called when > > > > > > available and interrupts are disabled for clients that request blocking > > > > > > transfers. > > > > > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > > > --- > > > > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > > > > include/linux/mailbox_controller.h | 4 ++++ > > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > > > > index 674b35f402f5..0eaf21259874 100644 > > > > > > --- a/drivers/mailbox/mailbox.c > > > > > > +++ b/drivers/mailbox/mailbox.c > > > > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > > > > unsigned long wait; > > > > > > int ret; > > > > > > > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > > > > + if (ret < 0) > > > > > > + tx_tick(chan, ret); > > > > > > + > > > > > > + return ret; > > > > > > + } > > > > > > + > > > > > This is hacky. I think we can do without busy waiting in atomic > > > > > context. You could queue locally while in atomic context and then > > > > > transfer in blocking mode. I don't think we should worry about the > > > > > 'throughput' as there already is no h/w rate control even with > > > > > busy-waiting. > > > > > > > > I actually tried to do that before I added this flushing mechanism. The > > > > problem is, like you said, one of rate control. As mentioned in the > > > > cover letter, the shared mailboxes implemented in tegra-hsp are used as > > > > RX and TX channels for the TCU, which is like a virtual UART. The TTY > > > > driver included as part of this series will use one of the mailboxes to > > > > transmit data that is written to the console. The problem is that if > > > > these transmissions are not rate-limited on the TTY driver side, the > > > > console will just keep writing data and eventually overflow the buffer > > > > that we have in the mailbox subsystem. > > > > > > > > The problem is that data comes in at a much higher rate than what we can > > > > output. This is especially true at boot when the TCU console takes over > > > > and the whole log buffer is dumped on it. > > > > > > > > So the only way to rate-limit is to either make mbox_send_message() > > > > block, but that can only be done in non-atomic context. The console, > > > > however, will always run in atomic context, so the only way to do rate- > > > > limiting is by busy looping. > > > > > > What I also tried before was to implement busy looping within the > > > ->send_data() callback of the driver so that we didn't have to put this > > > into the core. Unfortunately, however, the ->send_data() callback is > > > called under chan->lock, which means that from mbox_send_message() we > > > don't have a way to mark the transfer as done. In order to do that we'd > > > have to call mbox_chan_txdone(), but that ends up calling tx_tick() and > > > that in turn also attempts to take the chan->lock, which would cause a > > > deadlock. > > > > > > The explicit flushing is the best alternative that I could come up with. > > > I think it's not all that hacky, because it's very explicit about what's > > > going on and it has the nice side-effect that it will allow the mailbox > > > to work in interrupt driven mode if possible and only resorting to the > > > busy loop in atomic context. > > > > > > At this point I think I have explored all other options and I frankly > > > can't find a more proper way to achieve what we need here. Perhaps you > > > can think of additional ways to accomplish this? > > > > > Well, I would have a local ring buffer (array) of enough size to hold > > the characters and then have a task consuming data from that ring > > buffer by transmitting over mailbox. > > There's already such a ringbuffer in the printk code. To implement what > you suggest would effectively be creating a copy of that buffer because > we'd be allocating the buffer and the console code would just dump each > and every character in the logbuf into that ring buffer without rate- > limitation. > Well, the console assumes there exists an atomic path to put character on the bus, But because there isn't in case of tcu, we have to emulate that. Frankly I prefer the one-off driver jump some hoops, rather than implement exceptions in the api. BTW, there is already no rate-limitation because its all virtual - data is consumed as fast as possible. > To make matters worse, the ringbuffer would be empty most of the time > after the initial dump of the logbuf, so we'd be wasting all that buffer > space. > The idea is console and uart-ops both feed into this buffer and the only consumer thread runs the mailbox. > It just seems to me like we should be keeping the TCU driver as close as > possible to other UART drivers which also busy loop in order to rate- > limit what the console can write. Given the current mailbox framework it > is not possible to do that (in interrupt context), so an extension seems > like the most sensible option. > > Perhaps you'd be less concerned about such a change if it was perhaps > more explicit? Just throwing ideas around, I think something that could > also work is if we explicitly add a mbox_flush() function that would > basically be calling ->flush(). That way users of the mailbox can make > their requirement very explicit. I haven't actually tested that, but I > think it would work. Does that sound more acceptable to you? > I am happy to see features and bugfixes added to the api. What I am not eager about is supporting less than 100% legit and very platform specific usecases, especially when there is a work around. Thanks.
On Thu, Nov 22, 2018 at 10:07:09AM -0600, Jassi Brar wrote: > Hi Thierry, > > On Thu, Nov 22, 2018 at 2:47 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Wed, Nov 21, 2018 at 08:18:22PM -0600, Jassi Brar wrote: > > > On Wed, Nov 21, 2018 at 8:27 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > On Tue, Nov 20, 2018 at 04:29:07PM +0100, Thierry Reding wrote: > > > > > On Sat, Nov 17, 2018 at 11:27:17AM -0600, Jassi Brar wrote: > > > > > > On Mon, Nov 12, 2018 at 9:18 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > > > > > > > The mailbox framework supports blocking transfers via completions for > > > > > > > clients that can sleep. In order to support blocking transfers in cases > > > > > > > where the transmission is not permitted to sleep, add a new ->flush() > > > > > > > callback that controller drivers can implement to busy loop until the > > > > > > > transmission has been completed. This will automatically be called when > > > > > > > available and interrupts are disabled for clients that request blocking > > > > > > > transfers. > > > > > > > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > > > > --- > > > > > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > > > > > include/linux/mailbox_controller.h | 4 ++++ > > > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > > > > > index 674b35f402f5..0eaf21259874 100644 > > > > > > > --- a/drivers/mailbox/mailbox.c > > > > > > > +++ b/drivers/mailbox/mailbox.c > > > > > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > > > > > unsigned long wait; > > > > > > > int ret; > > > > > > > > > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > > > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > > > > > + if (ret < 0) > > > > > > > + tx_tick(chan, ret); > > > > > > > + > > > > > > > + return ret; > > > > > > > + } > > > > > > > + > > > > > > This is hacky. I think we can do without busy waiting in atomic > > > > > > context. You could queue locally while in atomic context and then > > > > > > transfer in blocking mode. I don't think we should worry about the > > > > > > 'throughput' as there already is no h/w rate control even with > > > > > > busy-waiting. > > > > > > > > > > I actually tried to do that before I added this flushing mechanism. The > > > > > problem is, like you said, one of rate control. As mentioned in the > > > > > cover letter, the shared mailboxes implemented in tegra-hsp are used as > > > > > RX and TX channels for the TCU, which is like a virtual UART. The TTY > > > > > driver included as part of this series will use one of the mailboxes to > > > > > transmit data that is written to the console. The problem is that if > > > > > these transmissions are not rate-limited on the TTY driver side, the > > > > > console will just keep writing data and eventually overflow the buffer > > > > > that we have in the mailbox subsystem. > > > > > > > > > > The problem is that data comes in at a much higher rate than what we can > > > > > output. This is especially true at boot when the TCU console takes over > > > > > and the whole log buffer is dumped on it. > > > > > > > > > > So the only way to rate-limit is to either make mbox_send_message() > > > > > block, but that can only be done in non-atomic context. The console, > > > > > however, will always run in atomic context, so the only way to do rate- > > > > > limiting is by busy looping. > > > > > > > > What I also tried before was to implement busy looping within the > > > > ->send_data() callback of the driver so that we didn't have to put this > > > > into the core. Unfortunately, however, the ->send_data() callback is > > > > called under chan->lock, which means that from mbox_send_message() we > > > > don't have a way to mark the transfer as done. In order to do that we'd > > > > have to call mbox_chan_txdone(), but that ends up calling tx_tick() and > > > > that in turn also attempts to take the chan->lock, which would cause a > > > > deadlock. > > > > > > > > The explicit flushing is the best alternative that I could come up with. > > > > I think it's not all that hacky, because it's very explicit about what's > > > > going on and it has the nice side-effect that it will allow the mailbox > > > > to work in interrupt driven mode if possible and only resorting to the > > > > busy loop in atomic context. > > > > > > > > At this point I think I have explored all other options and I frankly > > > > can't find a more proper way to achieve what we need here. Perhaps you > > > > can think of additional ways to accomplish this? > > > > > > > Well, I would have a local ring buffer (array) of enough size to hold > > > the characters and then have a task consuming data from that ring > > > buffer by transmitting over mailbox. > > > > There's already such a ringbuffer in the printk code. To implement what > > you suggest would effectively be creating a copy of that buffer because > > we'd be allocating the buffer and the console code would just dump each > > and every character in the logbuf into that ring buffer without rate- > > limitation. > > > Well, the console assumes there exists an atomic path to put character > on the bus, But because there isn't in case of tcu, we have to emulate > that. Frankly I prefer the one-off driver jump some hoops, rather than > implement exceptions in the api. I wouldn't have any objections to that if the hoops were reasonable ones. What you're asking me to do is basically implement a second copy of the logbuf. I don't call that a reasonable hoop to jump through. It is also not guaranteed to work properly because we can always end up with a situation where we produce more data than we can consume. Also, by providing this additional buffer we make things worse because the standard mechanisms of the logbuf are side-stepped. Typically the logbuf code will warn if it overflows. In our case we provide a buffer that the console can dump into, so instead of the logbuf overflowing and warning about it, we'd now overflow the mailbox buffer and we'd have to add extra code to warn about overflows. The console really only works because it assumes that the output driver will stall and thereby rate-limit. > BTW, there is already no rate-limitation because its all virtual - > data is consumed as fast as possible. No, that's not true. On the receiving end of the TX mailbox is a micro- processor that reads out the mailbox data. Once it has read the data it needs to clear the FULL bit so that the TCU driver can write more data. Even if the microprocessor on the receiving end did buffering (there is no indication that it does) it would eventually run out of buffer space and have to stall until it's clocked all the characters out of the physical UART. At that point no amount of buffering is going to save us and the only option is to stall, which, again, can currently only be done from the mailbox, because it is the only one that knows when it is busy. But it's also the one place where we can't because the framework doesn't allow it. > > To make matters worse, the ringbuffer would be empty most of the time > > after the initial dump of the logbuf, so we'd be wasting all that buffer > > space. > > > The idea is console and uart-ops both feed into this buffer and the > only consumer thread runs the mailbox. > > > It just seems to me like we should be keeping the TCU driver as close as > > possible to other UART drivers which also busy loop in order to rate- > > limit what the console can write. Given the current mailbox framework it > > is not possible to do that (in interrupt context), so an extension seems > > like the most sensible option. > > > > Perhaps you'd be less concerned about such a change if it was perhaps > > more explicit? Just throwing ideas around, I think something that could > > also work is if we explicitly add a mbox_flush() function that would > > basically be calling ->flush(). That way users of the mailbox can make > > their requirement very explicit. I haven't actually tested that, but I > > think it would work. Does that sound more acceptable to you? > > > I am happy to see features and bugfixes added to the api. What I am > not eager about is supporting less than 100% legit and very platform > specific usecases, especially when there is a work around. Look, I'd be willing to push this all into the driver, but the framework doesn't allow me to do that. If it was possible to run the state machine outside of mbox_send_message() then I'd be able to just move the busy loop or flush into the driver. But the locking is such that I can't do that because it will cause a deadlock. The ringbuffer workaround would be very brittle and if at all only work by accident, not to mention that it would require duplicating much of the logbuf logic. Also I don't consider usage in atomic context a very platform specific use-case, and I don't understand why I should be required to resort to some unreliable workaround rather than find a proper fix that guarantees proper operation. Thierry
On Thu, Nov 22, 2018 at 09:47:12AM +0100, Thierry Reding wrote: [...] > Perhaps you'd be less concerned about such a change if it was perhaps > more explicit? Just throwing ideas around, I think something that could > also work is if we explicitly add a mbox_flush() function that would > basically be calling ->flush(). That way users of the mailbox can make > their requirement very explicit. I haven't actually tested that, but I > think it would work. Does that sound more acceptable to you? I tried implementing the explicit flushing on top of this series and it would look roughly like the below. What do you think? Thierry --->8--- diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index 3e7e2c4358aa..fbdcc82a61ae 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -267,14 +267,6 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) unsigned long wait; int ret; - if (irqs_disabled() && chan->mbox->ops->flush) { - ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); - if (ret < 0) - tx_tick(chan, ret); - - return ret; - } - if (!chan->cl->tx_tout) /* wait forever */ wait = msecs_to_jiffies(3600000); else @@ -291,6 +283,34 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) } EXPORT_SYMBOL_GPL(mbox_send_message); +/** + * mbox_flush - flush a mailbox channel + * @chan: mailbox channel to flush + * @timeout: time, in milliseconds, to allow the flush operation to succeed + * + * Mailbox controllers that need to work in atomic context can implement the + * ->flush() callback to busy loop until a transmission has been completed. + * The implementation must call mbox_chan_txdone() upon success. Clients can + * call the mbox_flush() function at any time after mbox_send_message() to + * flush the transmission. After the function returns success, the mailbox + * transmission is guaranteed to have completed. + * + * Returns: 0 on success or a negative error code on failure. + */ +int mbox_flush(struct mbox_chan *chan, unsigned long timeout) +{ + int ret; + + if (!chan->mbox->ops->flush) + return -ENOTSUPP; + + ret = chan->mbox->ops->flush(chan, timeout); + if (ret < 0) + tx_tick(chan, ret); + + return ret; +} + /** * mbox_request_channel - Request a mailbox channel. * @cl: Identity of the client requesting the channel. diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c index f99c406cb3cc..e443f6a2ec4b 100644 --- a/drivers/mailbox/tegra-hsp.c +++ b/drivers/mailbox/tegra-hsp.c @@ -387,15 +387,13 @@ static int tegra_hsp_mailbox_send_data(struct mbox_chan *chan, void *data) tegra_hsp_channel_writel(&mb->channel, value, HSP_SM_SHRD_MBOX); - if (!irqs_disabled()) { - /* enable EMPTY interrupt for the shared mailbox */ - spin_lock_irqsave(&hsp->lock, flags); + /* enable EMPTY interrupt for the shared mailbox */ + spin_lock_irqsave(&hsp->lock, flags); - hsp->mask |= BIT(HSP_INT_EMPTY_SHIFT + mb->index); - tegra_hsp_writel(hsp, hsp->mask, HSP_INT_IE(hsp->shared_irq)); + hsp->mask |= BIT(HSP_INT_EMPTY_SHIFT + mb->index); + tegra_hsp_writel(hsp, hsp->mask, HSP_INT_IE(hsp->shared_irq)); - spin_unlock_irqrestore(&hsp->lock, flags); - } + spin_unlock_irqrestore(&hsp->lock, flags); return 0; } diff --git a/drivers/tty/serial/tegra-tcu.c b/drivers/tty/serial/tegra-tcu.c index 1d360cd03b18..59eaa13e169e 100644 --- a/drivers/tty/serial/tegra-tcu.c +++ b/drivers/tty/serial/tegra-tcu.c @@ -57,6 +57,7 @@ static void tegra_tcu_write_one(struct tegra_tcu *tcu, u32 value, value |= TCU_MBOX_NUM_BYTES(count); msg = (void *)(unsigned long)value; mbox_send_message(tcu->tx, msg); + mbox_flush(tcu->tx, 1000); } static void tegra_tcu_write(struct tegra_tcu *tcu, const char *s, @@ -184,9 +185,6 @@ static int tegra_tcu_probe(struct platform_device *pdev) return -ENOMEM; tcu->tx_client.dev = &pdev->dev; - tcu->tx_client.tx_block = true; - tcu->tx_client.tx_tout = 10000; - tcu->rx_client.dev = &pdev->dev; tcu->rx_client.rx_callback = tegra_tcu_receive; tcu->tx = mbox_request_channel_byname(&tcu->tx_client, "tx"); diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h index 44348710953f..faa7da3c9c8b 100644 --- a/include/linux/mailbox_client.h +++ b/include/linux/mailbox_client.h @@ -44,6 +44,7 @@ struct mbox_chan *mbox_request_channel_byname(struct mbox_client *cl, const char *name); struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index); int mbox_send_message(struct mbox_chan *chan, void *mssg); +int mbox_flush(struct mbox_chan *chan, unsigned long timeout); void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */ bool mbox_client_peek_data(struct mbox_chan *chan); /* atomic */ void mbox_free_channel(struct mbox_chan *chan); /* may sleep */
On Fri, Nov 23, 2018 at 12:17:00PM +0100, Thierry Reding wrote: > On Thu, Nov 22, 2018 at 09:47:12AM +0100, Thierry Reding wrote: > [...] > > Perhaps you'd be less concerned about such a change if it was perhaps > > more explicit? Just throwing ideas around, I think something that could > > also work is if we explicitly add a mbox_flush() function that would > > basically be calling ->flush(). That way users of the mailbox can make > > their requirement very explicit. I haven't actually tested that, but I > > think it would work. Does that sound more acceptable to you? > > I tried implementing the explicit flushing on top of this series and it > would look roughly like the below. What do you think? > > Thierry > > --->8--- > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c [...] > @@ -184,9 +185,6 @@ static int tegra_tcu_probe(struct platform_device *pdev) > return -ENOMEM; > > tcu->tx_client.dev = &pdev->dev; > - tcu->tx_client.tx_block = true; > - tcu->tx_client.tx_tout = 10000; > - tcu->rx_client.dev = &pdev->dev; Somehow this line ended up being removed in the diff, but it's actually required. Only tx_block and tx_tout should be removed in this hunk. Thierry
On 12/11/2018 15:18, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > The mailbox framework supports blocking transfers via completions for > clients that can sleep. In order to support blocking transfers in cases > where the transmission is not permitted to sleep, add a new ->flush() > callback that controller drivers can implement to busy loop until the > transmission has been completed. This will automatically be called when > available and interrupts are disabled for clients that request blocking > transfers. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/mailbox/mailbox.c | 8 ++++++++ > include/linux/mailbox_controller.h | 4 ++++ > 2 files changed, 12 insertions(+) > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > index 674b35f402f5..0eaf21259874 100644 > --- a/drivers/mailbox/mailbox.c > +++ b/drivers/mailbox/mailbox.c > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > unsigned long wait; > int ret; > > + if (irqs_disabled() && chan->mbox->ops->flush) { > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > + if (ret < 0) > + tx_tick(chan, ret); > + > + return ret; > + } It seems to me that if mbox_send_message() is called from an atomic context AND tx_block is true, then if 'flush' is not populated this should be an error condition as we do not wish to call wait_for_completion from an atomic context. I understand that there is some debate about adding such flush support, but irrespective of the above change, it seems to me that if the mbox_send_message() can be called from an atomic context (which it appears to), then it should be detecting if someone is trying to do so with 'tx_block' set as this should be an error. Furthermore, if the underlying mailbox driver can support sending a message from an atomic context and busy wait until it is done, surely the mailbox framework should provide a means to support this? Now it could be possible for the underlying mailbox driver to detect we are in an atomic context and if the 'tx_block' is set do the right thing by busy waiting until the message is sent. However, the problem with that is, that for the mbox_send_message() to ensure the right thing is done, it needs to check that 'tx_done' is set in the case of a blocking transfer in an atomic context. At that point you may as well add the flush operator as I think it is more implicit/clear. Cheers Jon
On Wed, Nov 28, 2018 at 09:43:29AM +0000, Jon Hunter wrote: > > On 12/11/2018 15:18, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > The mailbox framework supports blocking transfers via completions for > > clients that can sleep. In order to support blocking transfers in cases > > where the transmission is not permitted to sleep, add a new ->flush() > > callback that controller drivers can implement to busy loop until the > > transmission has been completed. This will automatically be called when > > available and interrupts are disabled for clients that request blocking > > transfers. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/mailbox/mailbox.c | 8 ++++++++ > > include/linux/mailbox_controller.h | 4 ++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > index 674b35f402f5..0eaf21259874 100644 > > --- a/drivers/mailbox/mailbox.c > > +++ b/drivers/mailbox/mailbox.c > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > unsigned long wait; > > int ret; > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > + if (ret < 0) > > + tx_tick(chan, ret); > > + > > + return ret; > > + } > > It seems to me that if mbox_send_message() is called from an atomic > context AND tx_block is true, then if 'flush' is not populated this > should be an error condition as we do not wish to call > wait_for_completion from an atomic context. > > I understand that there is some debate about adding such flush support, > but irrespective of the above change, it seems to me that if the > mbox_send_message() can be called from an atomic context (which it > appears to), then it should be detecting if someone is trying to do so > with 'tx_block' set as this should be an error. > > Furthermore, if the underlying mailbox driver can support sending a > message from an atomic context and busy wait until it is done, surely > the mailbox framework should provide a means to support this? > > Now it could be possible for the underlying mailbox driver to detect we > are in an atomic context and if the 'tx_block' is set do the right thing > by busy waiting until the message is sent. However, the problem with > that is, that for the mbox_send_message() to ensure the right thing is > done, it needs to check that 'tx_done' is set in the case of a blocking > transfer in an atomic context. At that point you may as well add the > flush operator as I think it is more implicit/clear. Heh, interesting timing, I just sent out v3 of this series with a slightly different solution. Basically what v3 implements is explicit flushing of the mailbox via a new function called mbox_flush(). I originally liked the idea of "hiding" the flush operation in the mbox_send_message() function, but the more I look at the new explicit flush solution, the more I prefer it. One reason why I prefer it is because it no longer has the slightly odd "irqs_disabled()" check that always made me a bit uneasy. The other advantage of the explicit flush is that it becomes completely opt-in for both mailbox providers and consumers. Furthermore the consumer should always know if it can be called in atomic context or not, so none of these code paths should have to be "clever" and check at runtime whether or not interrupts are enabled. If the consumer knows that it will be called in atomic context it can just unconditionally flush the mailbox. Also, it nicely addresses the concern about erroring out if the provider doesn't support flushing. The new mbox_flush() API returns -ENOTSUPP if called on a mailbox channel provided by a driver that doesn't implement ->flush(). Thierry
On Wed, Nov 28, 2018 at 3:43 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 12/11/2018 15:18, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > The mailbox framework supports blocking transfers via completions for > > clients that can sleep. In order to support blocking transfers in cases > > where the transmission is not permitted to sleep, add a new ->flush() > > callback that controller drivers can implement to busy loop until the > > transmission has been completed. This will automatically be called when > > available and interrupts are disabled for clients that request blocking > > transfers. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/mailbox/mailbox.c | 8 ++++++++ > > include/linux/mailbox_controller.h | 4 ++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > index 674b35f402f5..0eaf21259874 100644 > > --- a/drivers/mailbox/mailbox.c > > +++ b/drivers/mailbox/mailbox.c > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > unsigned long wait; > > int ret; > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > + if (ret < 0) > > + tx_tick(chan, ret); > > + > > + return ret; > > + } > > It seems to me that if mbox_send_message() is called from an atomic > context AND tx_block is true, then if 'flush' is not populated this > should be an error condition as we do not wish to call > wait_for_completion from an atomic context. > > I understand that there is some debate about adding such flush support, > but irrespective of the above change, it seems to me that if the > mbox_send_message() can be called from an atomic context (which it > appears to), then it should be detecting if someone is trying to do so > with 'tx_block' set as this should be an error. > Layers within kernel space have to trust each other. A badly written client can break the consumer in so many ways, we can not catch every possibility. > Furthermore, if the underlying mailbox driver can support sending a > message from an atomic context and busy wait until it is done, surely > the mailbox framework should provide a means to support this? > Being able to put the message on bus in atomic context is a feature - which we do support. But busy-waiting in a loop is not a feature, and we don't want to encourage that. Cheers!
On Wed, Nov 28, 2018 at 11:23:36PM -0600, Jassi Brar wrote: > On Wed, Nov 28, 2018 at 3:43 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > > > On 12/11/2018 15:18, Thierry Reding wrote: > > > From: Thierry Reding <treding@nvidia.com> > > > > > > The mailbox framework supports blocking transfers via completions for > > > clients that can sleep. In order to support blocking transfers in cases > > > where the transmission is not permitted to sleep, add a new ->flush() > > > callback that controller drivers can implement to busy loop until the > > > transmission has been completed. This will automatically be called when > > > available and interrupts are disabled for clients that request blocking > > > transfers. > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > include/linux/mailbox_controller.h | 4 ++++ > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > index 674b35f402f5..0eaf21259874 100644 > > > --- a/drivers/mailbox/mailbox.c > > > +++ b/drivers/mailbox/mailbox.c > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > unsigned long wait; > > > int ret; > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > + if (ret < 0) > > > + tx_tick(chan, ret); > > > + > > > + return ret; > > > + } > > > > It seems to me that if mbox_send_message() is called from an atomic > > context AND tx_block is true, then if 'flush' is not populated this > > should be an error condition as we do not wish to call > > wait_for_completion from an atomic context. > > > > I understand that there is some debate about adding such flush support, > > but irrespective of the above change, it seems to me that if the > > mbox_send_message() can be called from an atomic context (which it > > appears to), then it should be detecting if someone is trying to do so > > with 'tx_block' set as this should be an error. > > > Layers within kernel space have to trust each other. A badly written > client can break the consumer in so many ways, we can not catch every > possibility. > > > Furthermore, if the underlying mailbox driver can support sending a > > message from an atomic context and busy wait until it is done, surely > > the mailbox framework should provide a means to support this? > > > Being able to put the message on bus in atomic context is a feature - > which we do support. But busy-waiting in a loop is not a feature, and > we don't want to encourage that. I agree that in generally busy-waiting is a bad idea and shouldn't be encouraged. However, I also think that an exception proves the rule. If you look at the console drivers in drivers/tty/serial, all of them will busy loop prior to or after sending a character. This is pretty much part of the API and as such busy-looping is very much a feature. The reason why this is done is because on one hand we have an atomic context and on the other hand we want to make sure that all characters actually make it to the console when we print them. As an example how this can break, I've taken your suggestion to implement a producer/consumer mode in the TCU driver where the console write function will just stash characters into a circular buffer and a work queue will then use mbox_send_message() to drain the circular buffer. While this does work on the surface, I was able to concern both of the issues that I was concerned about: 1) it is easy to overflow the circular buffer by just dumping enough data at once to the console and 2) when a crash happens, everything in the kernel stops, including the consumer workqueue that is supposed to drain the circular buffer and flush messages to the TCU. The result is that, in my particular case, the boot log will just stop at a random place in the middle of messages from much earlier in the boot because the TCU hasn't caught up yet and there's a lot of characters still in the circular buffer. Now, 1) can be mitigated by increasing the circular buffer size. A value that seems to give reliably good results in 2 << CONFIG_LOG_BUF_SHIFT. I thought that I could also mitigate 2) by busy looping in the TCU driver, but it turns out that that doesn't work. The reason is that since we are in atomic context with all interrupts disabled, the mailbox won't ever consume any new characters, so the read pointer in the circular buffer won't increment, leaving me with no condition upon which to loop that would work. Given that 2) can't be solved makes this implementation of the TCU completely useless as a console. All of that said, have you had a chance to look at my other proposed solution for this? I sent it out yesterday, but you can also find the series here: http://patchwork.ozlabs.org/project/linux-tegra/list/?series=78477 The difference to the earlier versions is that the flushing is now explicit. I think this combines the best of both worlds. On one hand it makes the mechanism completely opt-in, so nothing gets hidden in the regular functions. On the other hand, it allows clients to make use of this functionality very explicitly. A client that doesn't call the mbox_flush() function will just not busy-loop. But clients that need to make sure messages are flushed in atomic contexts can now do that. Does that sound like a more acceptable solution to you? We could even go and add documentation to mbox_flush() that it should only be used under special circumstances. If you still think that's a bad idea, do you have any other suggestions on how to move forward? Thierry
On Thu, Nov 29, 2018 at 9:23 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Nov 28, 2018 at 11:23:36PM -0600, Jassi Brar wrote: > > On Wed, Nov 28, 2018 at 3:43 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > > > > > > On 12/11/2018 15:18, Thierry Reding wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > The mailbox framework supports blocking transfers via completions for > > > > clients that can sleep. In order to support blocking transfers in cases > > > > where the transmission is not permitted to sleep, add a new ->flush() > > > > callback that controller drivers can implement to busy loop until the > > > > transmission has been completed. This will automatically be called when > > > > available and interrupts are disabled for clients that request blocking > > > > transfers. > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > --- > > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > > include/linux/mailbox_controller.h | 4 ++++ > > > > 2 files changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > > index 674b35f402f5..0eaf21259874 100644 > > > > --- a/drivers/mailbox/mailbox.c > > > > +++ b/drivers/mailbox/mailbox.c > > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > > unsigned long wait; > > > > int ret; > > > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > > + if (ret < 0) > > > > + tx_tick(chan, ret); > > > > + > > > > + return ret; > > > > + } > > > > > > It seems to me that if mbox_send_message() is called from an atomic > > > context AND tx_block is true, then if 'flush' is not populated this > > > should be an error condition as we do not wish to call > > > wait_for_completion from an atomic context. > > > > > > I understand that there is some debate about adding such flush support, > > > but irrespective of the above change, it seems to me that if the > > > mbox_send_message() can be called from an atomic context (which it > > > appears to), then it should be detecting if someone is trying to do so > > > with 'tx_block' set as this should be an error. > > > > > Layers within kernel space have to trust each other. A badly written > > client can break the consumer in so many ways, we can not catch every > > possibility. > > > > > Furthermore, if the underlying mailbox driver can support sending a > > > message from an atomic context and busy wait until it is done, surely > > > the mailbox framework should provide a means to support this? > > > > > Being able to put the message on bus in atomic context is a feature - > > which we do support. But busy-waiting in a loop is not a feature, and > > we don't want to encourage that. > > I agree that in generally busy-waiting is a bad idea and shouldn't be > encouraged. However, I also think that an exception proves the rule. If > you look at the console drivers in drivers/tty/serial, all of them will > busy loop prior to or after sending a character. This is pretty much > part of the API and as such busy-looping is very much a feature. > > The reason why this is done is because on one hand we have an atomic > context and on the other hand we want to make sure that all characters > actually make it to the console when we print them. > > As an example how this can break, I've taken your suggestion to > implement a producer/consumer mode in the TCU driver where the console > write function will just stash characters into a circular buffer and a > work queue will then use mbox_send_message() to drain the circular > buffer. While this does work on the surface, I was able to concern both > of the issues that I was concerned about: 1) it is easy to overflow the > circular buffer by just dumping enough data at once to the console and > 2) when a crash happens, everything in the kernel stops, including the > consumer workqueue that is supposed to drain the circular buffer and > flush messages to the TCU. The result is that, in my particular case, > the boot log will just stop at a random place in the middle of messages > from much earlier in the boot because the TCU hasn't caught up yet and > there's a lot of characters still in the circular buffer. > > Now, 1) can be mitigated by increasing the circular buffer size. A value > that seems to give reliably good results in 2 << CONFIG_LOG_BUF_SHIFT. > Yes please. > I thought that I could also mitigate 2) by busy looping in the TCU driver, > but it turns out that that doesn't work. The reason is that since we are > in atomic context with all interrupts disabled, the mailbox won't ever > consume any new characters, so the read pointer in the circular buffer > won't increment, leaving me with no condition upon which to loop that > would work. > So you want to be able to rely on an emulated console (running on a totally different subsystem) to dump development-phase early-boot logs? At the cost of legitimizing busy looping in atomic context - one random driver messing up the api for ever. Maybe you could have the ring buffer in some shmem and only pass the number of valid characters in it, to the remote? > > http://patchwork.ozlabs.org/project/linux-tegra/list/?series=78477 > > The difference to the earlier versions is that the flushing is now > explicit. I think this combines the best of both worlds. On one hand it > makes the mechanism completely opt-in, so nothing gets hidden in the > regular functions. On the other hand, it allows clients to make use of > this functionality very explicitly. A client that doesn't call the > mbox_flush() function will just not busy-loop. But clients that need to > make sure messages are flushed in atomic contexts can now do that. Does > that sound like a more acceptable solution to you? We could even go and > add documentation to mbox_flush() that it should only be used under > special circumstances. > > If you still think that's a bad idea, do you have any other suggestions > on how to move forward? > It would help if other maintainers chime in if a subsystem should support busy-wait in atomic context for a one-off driver. Regards.
On 07/12/2018 11.26, Jassi Brar wrote: >> I thought that I could also mitigate 2) by busy looping in the TCU driver, >> but it turns out that that doesn't work. The reason is that since we are >> in atomic context with all interrupts disabled, the mailbox won't ever >> consume any new characters, so the read pointer in the circular buffer >> won't increment, leaving me with no condition upon which to loop that >> would work. >> > So you want to be able to rely on an emulated console (running on a > totally different subsystem) to dump development-phase early-boot > logs? At the cost of legitimizing busy looping in atomic context - one > random driver messing up the api for ever. Maybe you could have the > ring buffer in some shmem and only pass the number of valid characters > in it, to the remote? > I would like to note that this is the one and only console interface that exists on these systems, for both development phase and production. Other UARTs are not externally accessible on the systems, or they are comparatively difficult to access as they aren't intended to be used as consoles in the system design. The combination of hardware and firmware implementing the console is black box from software's point of view (similarly to any other UART HW). The interface has also been fixed at an early system design phase, as there are many operating systems running on these boards, each with their own drivers. Cheers, Mikko
On Thu, Dec 06, 2018 at 11:56:25PM -0600, Jassi Brar wrote: > On Thu, Nov 29, 2018 at 9:23 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Wed, Nov 28, 2018 at 11:23:36PM -0600, Jassi Brar wrote: > > > On Wed, Nov 28, 2018 at 3:43 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > > > > > > > > > On 12/11/2018 15:18, Thierry Reding wrote: > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > > > The mailbox framework supports blocking transfers via completions for > > > > > clients that can sleep. In order to support blocking transfers in cases > > > > > where the transmission is not permitted to sleep, add a new ->flush() > > > > > callback that controller drivers can implement to busy loop until the > > > > > transmission has been completed. This will automatically be called when > > > > > available and interrupts are disabled for clients that request blocking > > > > > transfers. > > > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > > --- > > > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > > > include/linux/mailbox_controller.h | 4 ++++ > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > > > index 674b35f402f5..0eaf21259874 100644 > > > > > --- a/drivers/mailbox/mailbox.c > > > > > +++ b/drivers/mailbox/mailbox.c > > > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > > > unsigned long wait; > > > > > int ret; > > > > > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > > > + if (ret < 0) > > > > > + tx_tick(chan, ret); > > > > > + > > > > > + return ret; > > > > > + } > > > > > > > > It seems to me that if mbox_send_message() is called from an atomic > > > > context AND tx_block is true, then if 'flush' is not populated this > > > > should be an error condition as we do not wish to call > > > > wait_for_completion from an atomic context. > > > > > > > > I understand that there is some debate about adding such flush support, > > > > but irrespective of the above change, it seems to me that if the > > > > mbox_send_message() can be called from an atomic context (which it > > > > appears to), then it should be detecting if someone is trying to do so > > > > with 'tx_block' set as this should be an error. > > > > > > > Layers within kernel space have to trust each other. A badly written > > > client can break the consumer in so many ways, we can not catch every > > > possibility. > > > > > > > Furthermore, if the underlying mailbox driver can support sending a > > > > message from an atomic context and busy wait until it is done, surely > > > > the mailbox framework should provide a means to support this? > > > > > > > Being able to put the message on bus in atomic context is a feature - > > > which we do support. But busy-waiting in a loop is not a feature, and > > > we don't want to encourage that. > > > > I agree that in generally busy-waiting is a bad idea and shouldn't be > > encouraged. However, I also think that an exception proves the rule. If > > you look at the console drivers in drivers/tty/serial, all of them will > > busy loop prior to or after sending a character. This is pretty much > > part of the API and as such busy-looping is very much a feature. > > > > The reason why this is done is because on one hand we have an atomic > > context and on the other hand we want to make sure that all characters > > actually make it to the console when we print them. > > > > As an example how this can break, I've taken your suggestion to > > implement a producer/consumer mode in the TCU driver where the console > > write function will just stash characters into a circular buffer and a > > work queue will then use mbox_send_message() to drain the circular > > buffer. While this does work on the surface, I was able to concern both > > of the issues that I was concerned about: 1) it is easy to overflow the > > circular buffer by just dumping enough data at once to the console and > > 2) when a crash happens, everything in the kernel stops, including the > > consumer workqueue that is supposed to drain the circular buffer and > > flush messages to the TCU. The result is that, in my particular case, > > the boot log will just stop at a random place in the middle of messages > > from much earlier in the boot because the TCU hasn't caught up yet and > > there's a lot of characters still in the circular buffer. > > > > Now, 1) can be mitigated by increasing the circular buffer size. A value > > that seems to give reliably good results in 2 << CONFIG_LOG_BUF_SHIFT. > > > Yes please. As I explained elsewhere, I actually went and implemented this. But given the nature of buffering, this ends up making the TCU completely useless as a console because in case of a crash, the system will stop working with a large number of characters still stuck in the buffer. And that's especially bad in case of a crash because those last characters that get stuck in the buffer are the most relevant ones because they contain the stack dump. > > I thought that I could also mitigate 2) by busy looping in the TCU driver, > > but it turns out that that doesn't work. The reason is that since we are > > in atomic context with all interrupts disabled, the mailbox won't ever > > consume any new characters, so the read pointer in the circular buffer > > won't increment, leaving me with no condition upon which to loop that > > would work. > > > So you want to be able to rely on an emulated console (running on a > totally different subsystem) to dump development-phase early-boot > logs? At the cost of legitimizing busy looping in atomic context - one > random driver messing up the api for ever. Maybe you could have the > ring buffer in some shmem and only pass the number of valid characters > in it, to the remote? First of all, this is not about development-phase early-boot messages. We're talking about the system console here. This is what everyone will want to be using when developing on this device. Sure at some point you may end up with a system that works and you can have the console on the network or an attached display or whatever, but even then you may still want to attach to the console if you ever run into issues where the system doesn't come up. Secondly, I don't understand why you think this is an emulated console. The way that this works is that there's a microprocessor in the system that interacts with other microprocessors via shared mailboxes to receive log messages. That microprocessor collects these log messages and outputs them on a physical UART via multiplexed streams. The host system can connect to that physical UART and demultiplex these streams to get the individual log messages from each of the components in the system. Lastly, I don't understand why you think we're messing up the API here. The proposal in this series doesn't even change any of the API, but it makes it aware of the state of interrupts internally so that it can do the right thing depending on the call stack. The other proposal, in v3, is more explicit in that it adds new API to flush the mailbox. The new API is completely optional and I even offered to document it as being discouraged because it involves busy looping. At the same time it does solve a real problem and it doesn't impact any existing mailbox drivers nor any of their users (because it is completely opt-in). While I can somewhat relate to your reluctance to extend the API, I do not see a way around it. Sure, you could decide that this is something that Linux just won't support, but that would be a first. I'm not aware of any cases where a concious decision was ever made not to support a feature because it didn't fit into any existing frameworks. Typically we deal with this by improving things so that we can support the additional use-cases. It's really one of the strong points of Linux. I'm open to any suggestions on how this could be done better. You had already suggested the ring buffer and I did go and implement it, but it only confirmed the concerns that I've had with it all along. I realize that at this point I may be blind to other obvious solutions, but I've done my best to come up with alternatives and none of them work. If you have any other ideas, please do share them and I will investigate. > > http://patchwork.ozlabs.org/project/linux-tegra/list/?series=78477 > > > > The difference to the earlier versions is that the flushing is now > > explicit. I think this combines the best of both worlds. On one hand it > > makes the mechanism completely opt-in, so nothing gets hidden in the > > regular functions. On the other hand, it allows clients to make use of > > this functionality very explicitly. A client that doesn't call the > > mbox_flush() function will just not busy-loop. But clients that need to > > make sure messages are flushed in atomic contexts can now do that. Does > > that sound like a more acceptable solution to you? We could even go and > > add documentation to mbox_flush() that it should only be used under > > special circumstances. > > > > If you still think that's a bad idea, do you have any other suggestions > > on how to move forward? > > > It would help if other maintainers chime in if a subsystem should > support busy-wait in atomic context for a one-off driver. Just as an additional note: it's not the driver that actually requires the busy looping, it's the use-case (i.e. the consumer). The driver is working perfectly fine with interrupts enabled, it's just that in the particular use-case of the console that we have no way of detecting if or when an interrupt occurred. Greg, any ideas on how we can move forward here? For reasons given elsewhere in this thread I understand that there is no way to make the console code run in non-atomic context. Have you ever run into a case where the console driver couldn't busy-loop? Were there any solutions to this? I've looked through quite a few drivers and they all end up with a busy loop, waiting for the transmission FIFO to become empty. There are also a few implementations for hypervisors that call back into some firmware in order to send the characters, but I expect those to do the busy looping in the firmware. Perhaps the most prominent case that I came across and that is quite similar to this discussion is netconsole. There's a lot of code in the network layer that exists only to allow using a network device for outputting a console. I don't think the changes to the mailbox framework are anywhere near as complicated. Thierry
On Fri, Dec 07, 2018 at 12:32:45PM +0100, Thierry Reding wrote: > Greg, > > any ideas on how we can move forward here? For reasons given elsewhere > in this thread I understand that there is no way to make the console > code run in non-atomic context. Have you ever run into a case where the > console driver couldn't busy-loop? Were there any solutions to this? I don't know of any such cases, hardware sucks at times, and we just have to deal with getting it to work by doing stuff like this :( > I've looked through quite a few drivers and they all end up with a busy > loop, waiting for the transmission FIFO to become empty. There are also > a few implementations for hypervisors that call back into some firmware > in order to send the characters, but I expect those to do the busy > looping in the firmware. busy loops are ok here, as you point out. > Perhaps the most prominent case that I came across and that is quite > similar to this discussion is netconsole. There's a lot of code in the > network layer that exists only to allow using a network device for > outputting a console. I don't think the changes to the mailbox > framework are anywhere near as complicated. Neither do I, I have no objection to your changes at all. thanks, greg k-h
On Fri, Dec 7, 2018 at 11:50 AM Mikko Perttunen <cyndis@kapsi.fi> wrote: > > On 07/12/2018 11.26, Jassi Brar wrote: > >> I thought that I could also mitigate 2) by busy looping in the TCU driver, > >> but it turns out that that doesn't work. The reason is that since we are > >> in atomic context with all interrupts disabled, the mailbox won't ever > >> consume any new characters, so the read pointer in the circular buffer > >> won't increment, leaving me with no condition upon which to loop that > >> would work. > >> > > So you want to be able to rely on an emulated console (running on a > > totally different subsystem) to dump development-phase early-boot > > logs? At the cost of legitimizing busy looping in atomic context - one > > random driver messing up the api for ever. Maybe you could have the > > ring buffer in some shmem and only pass the number of valid characters > > in it, to the remote? > > > > I would like to note that this is the one and only console interface > that exists on these systems, for both development phase and production. > Other UARTs are not externally accessible on the systems, or they are > comparatively difficult to access as they aren't intended to be used as > consoles in the system design. > The combination of hardware and firmware > implementing the console is black box from software's point of view > (similarly to any other UART HW). The interface has also been fixed at > an early system design phase, as there are many operating systems > running on these boards, each with their own drivers. > That is the saddest part - someone, who writes test cases for the h/w team and with almost no knowledge of how OSes work, decides how the firmware is going to work and calls it done. Then the Linux is left to deal with the mess as we "can't do anything about it".
On Fri, Dec 7, 2018 at 5:02 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Dec 06, 2018 at 11:56:25PM -0600, Jassi Brar wrote: > > On Thu, Nov 29, 2018 at 9:23 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Wed, Nov 28, 2018 at 11:23:36PM -0600, Jassi Brar wrote: > > > > On Wed, Nov 28, 2018 at 3:43 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > > > > > > > > > > > > On 12/11/2018 15:18, Thierry Reding wrote: > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > > > > > The mailbox framework supports blocking transfers via completions for > > > > > > clients that can sleep. In order to support blocking transfers in cases > > > > > > where the transmission is not permitted to sleep, add a new ->flush() > > > > > > callback that controller drivers can implement to busy loop until the > > > > > > transmission has been completed. This will automatically be called when > > > > > > available and interrupts are disabled for clients that request blocking > > > > > > transfers. > > > > > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > > > --- > > > > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > > > > include/linux/mailbox_controller.h | 4 ++++ > > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > > > > index 674b35f402f5..0eaf21259874 100644 > > > > > > --- a/drivers/mailbox/mailbox.c > > > > > > +++ b/drivers/mailbox/mailbox.c > > > > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > > > > unsigned long wait; > > > > > > int ret; > > > > > > > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > > > > + if (ret < 0) > > > > > > + tx_tick(chan, ret); > > > > > > + > > > > > > + return ret; > > > > > > + } > > > > > > > > > > It seems to me that if mbox_send_message() is called from an atomic > > > > > context AND tx_block is true, then if 'flush' is not populated this > > > > > should be an error condition as we do not wish to call > > > > > wait_for_completion from an atomic context. > > > > > > > > > > I understand that there is some debate about adding such flush support, > > > > > but irrespective of the above change, it seems to me that if the > > > > > mbox_send_message() can be called from an atomic context (which it > > > > > appears to), then it should be detecting if someone is trying to do so > > > > > with 'tx_block' set as this should be an error. > > > > > > > > > Layers within kernel space have to trust each other. A badly written > > > > client can break the consumer in so many ways, we can not catch every > > > > possibility. > > > > > > > > > Furthermore, if the underlying mailbox driver can support sending a > > > > > message from an atomic context and busy wait until it is done, surely > > > > > the mailbox framework should provide a means to support this? > > > > > > > > > Being able to put the message on bus in atomic context is a feature - > > > > which we do support. But busy-waiting in a loop is not a feature, and > > > > we don't want to encourage that. > > > > > > I agree that in generally busy-waiting is a bad idea and shouldn't be > > > encouraged. However, I also think that an exception proves the rule. If > > > you look at the console drivers in drivers/tty/serial, all of them will > > > busy loop prior to or after sending a character. This is pretty much > > > part of the API and as such busy-looping is very much a feature. > > > > > > The reason why this is done is because on one hand we have an atomic > > > context and on the other hand we want to make sure that all characters > > > actually make it to the console when we print them. > > > > > > As an example how this can break, I've taken your suggestion to > > > implement a producer/consumer mode in the TCU driver where the console > > > write function will just stash characters into a circular buffer and a > > > work queue will then use mbox_send_message() to drain the circular > > > buffer. While this does work on the surface, I was able to concern both > > > of the issues that I was concerned about: 1) it is easy to overflow the > > > circular buffer by just dumping enough data at once to the console and > > > 2) when a crash happens, everything in the kernel stops, including the > > > consumer workqueue that is supposed to drain the circular buffer and > > > flush messages to the TCU. The result is that, in my particular case, > > > the boot log will just stop at a random place in the middle of messages > > > from much earlier in the boot because the TCU hasn't caught up yet and > > > there's a lot of characters still in the circular buffer. > > > > > > Now, 1) can be mitigated by increasing the circular buffer size. A value > > > that seems to give reliably good results in 2 << CONFIG_LOG_BUF_SHIFT. > > > > > Yes please. > > As I explained elsewhere, I actually went and implemented this. But > given the nature of buffering, this ends up making the TCU completely > useless as a console because in case of a crash, the system will stop > working with a large number of characters still stuck in the buffer. > And that's especially bad in case of a crash because those last > characters that get stuck in the buffer are the most relevant ones > because they contain the stack dump. > I don't question the utility of TCU. I just wonder if mailbox api should provide a backdoor for atomic busy-wait in order to support a sub-optimal hw+fw design. > > > I thought that I could also mitigate 2) by busy looping in the TCU driver, > > > but it turns out that that doesn't work. The reason is that since we are > > > in atomic context with all interrupts disabled, the mailbox won't ever > > > consume any new characters, so the read pointer in the circular buffer > > > won't increment, leaving me with no condition upon which to loop that > > > would work. > > > > > So you want to be able to rely on an emulated console (running on a > > totally different subsystem) to dump development-phase early-boot > > logs? At the cost of legitimizing busy looping in atomic context - one > > random driver messing up the api for ever. Maybe you could have the > > ring buffer in some shmem and only pass the number of valid characters > > in it, to the remote? > > First of all, this is not about development-phase early-boot messages. > We're talking about the system console here. This is what everyone will > want to be using when developing on this device. Sure at some point you > may end up with a system that works and you can have the console on the > network or an attached display or whatever, but even then you may still > want to attach to the console if you ever run into issues where the > system doesn't come up. > > Secondly, I don't understand why you think this is an emulated console. > It is emulated/virtual because Linux doesn't put characters on a physical bus. The data is simply handed forward to a remote entity. > Lastly, I don't understand why you think we're messing up the API here. > The proposal in this series doesn't even change any of the API, but it > makes it aware of the state of interrupts internally so that it can do > the right thing depending on the call stack. The other proposal, in v3, > is more explicit in that it adds new API to flush the mailbox. The new > API is completely optional and I even offered to document it as being > discouraged because it involves busy looping. At the same time it does > solve a real problem and it doesn't impact any existing mailbox drivers > nor any of their users (because it is completely opt-in). > The 'flush' api is going to have no use other than implement busy-waits. I am afraid people are simply going to take it easy and copy the busy-waits from the test/verification codes. "discouraging" seldom works because the developer comes with the failproof excuse "the h/w doesn't provide any other way". Frankly I would like to push back on such designs. Anyways, let us add the new 'flush' api. Thanks.
On Sat, Dec 08, 2018 at 11:21:41AM +0530, Jassi Brar wrote: > On Fri, Dec 7, 2018 at 11:50 AM Mikko Perttunen <cyndis@kapsi.fi> wrote: > > > > On 07/12/2018 11.26, Jassi Brar wrote: > > >> I thought that I could also mitigate 2) by busy looping in the TCU driver, > > >> but it turns out that that doesn't work. The reason is that since we are > > >> in atomic context with all interrupts disabled, the mailbox won't ever > > >> consume any new characters, so the read pointer in the circular buffer > > >> won't increment, leaving me with no condition upon which to loop that > > >> would work. > > >> > > > So you want to be able to rely on an emulated console (running on a > > > totally different subsystem) to dump development-phase early-boot > > > logs? At the cost of legitimizing busy looping in atomic context - one > > > random driver messing up the api for ever. Maybe you could have the > > > ring buffer in some shmem and only pass the number of valid characters > > > in it, to the remote? > > > > > > > I would like to note that this is the one and only console interface > > that exists on these systems, for both development phase and production. > > Other UARTs are not externally accessible on the systems, or they are > > comparatively difficult to access as they aren't intended to be used as > > consoles in the system design. > > > The combination of hardware and firmware > > implementing the console is black box from software's point of view > > (similarly to any other UART HW). The interface has also been fixed at > > an early system design phase, as there are many operating systems > > running on these boards, each with their own drivers. > > > That is the saddest part - someone, who writes test cases for the h/w > team and with almost no knowledge of how OSes work, decides how the > firmware is going to work and calls it done. Then the Linux is left to > deal with the mess as we "can't do anything about it". That is totally normal, and is how hardware has been almost _always_ been designed and implemented. Nothing new here at all, it's just the life us kernel developers get used to very quickly as it is our job to make that hardware work and appear to userspace as something sane and universal. Sorry, greg k-h
On Sat, Dec 8, 2018 at 2:50 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Sat, Dec 08, 2018 at 11:21:41AM +0530, Jassi Brar wrote: > > On Fri, Dec 7, 2018 at 11:50 AM Mikko Perttunen <cyndis@kapsi.fi> wrote: > > > > > > On 07/12/2018 11.26, Jassi Brar wrote: > > > >> I thought that I could also mitigate 2) by busy looping in the TCU driver, > > > >> but it turns out that that doesn't work. The reason is that since we are > > > >> in atomic context with all interrupts disabled, the mailbox won't ever > > > >> consume any new characters, so the read pointer in the circular buffer > > > >> won't increment, leaving me with no condition upon which to loop that > > > >> would work. > > > >> > > > > So you want to be able to rely on an emulated console (running on a > > > > totally different subsystem) to dump development-phase early-boot > > > > logs? At the cost of legitimizing busy looping in atomic context - one > > > > random driver messing up the api for ever. Maybe you could have the > > > > ring buffer in some shmem and only pass the number of valid characters > > > > in it, to the remote? > > > > > > > > > > I would like to note that this is the one and only console interface > > > that exists on these systems, for both development phase and production. > > > Other UARTs are not externally accessible on the systems, or they are > > > comparatively difficult to access as they aren't intended to be used as > > > consoles in the system design. > > > > > The combination of hardware and firmware > > > implementing the console is black box from software's point of view > > > (similarly to any other UART HW). The interface has also been fixed at > > > an early system design phase, as there are many operating systems > > > running on these boards, each with their own drivers. > > > > > That is the saddest part - someone, who writes test cases for the h/w > > team and with almost no knowledge of how OSes work, decides how the > > firmware is going to work and calls it done. Then the Linux is left to > > deal with the mess as we "can't do anything about it". > > That is totally normal, and is how hardware has been almost _always_ > been designed and implemented. Nothing new here at all, it's just the > life us kernel developers get used to very quickly as it is our job to > make that hardware work and appear to userspace as something sane and > universal. > Hardware yes, we can't really change much after the fact. In this case the issue arises from the firmware - TCU's mailbox protocol implementation. Which usually is just another image loaded onto the remote master during cold-boot, and should actually be not that hard to fix/change. Even if other OSes (really?) have adapted, a pushback right now will help fix atleast the next version, otherwise the api designer will never know what s/he is doing wrong. Anyways, thanks for chiming in. I will pull the patchset. Thanks.
On Sat, Dec 08, 2018 at 11:39:05AM +0530, Jassi Brar wrote: > On Fri, Dec 7, 2018 at 5:02 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Thu, Dec 06, 2018 at 11:56:25PM -0600, Jassi Brar wrote: > > > On Thu, Nov 29, 2018 at 9:23 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > On Wed, Nov 28, 2018 at 11:23:36PM -0600, Jassi Brar wrote: > > > > > On Wed, Nov 28, 2018 at 3:43 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > On 12/11/2018 15:18, Thierry Reding wrote: > > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > > > > > > > The mailbox framework supports blocking transfers via completions for > > > > > > > clients that can sleep. In order to support blocking transfers in cases > > > > > > > where the transmission is not permitted to sleep, add a new ->flush() > > > > > > > callback that controller drivers can implement to busy loop until the > > > > > > > transmission has been completed. This will automatically be called when > > > > > > > available and interrupts are disabled for clients that request blocking > > > > > > > transfers. > > > > > > > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > > > > --- > > > > > > > drivers/mailbox/mailbox.c | 8 ++++++++ > > > > > > > include/linux/mailbox_controller.h | 4 ++++ > > > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > > > > > > > index 674b35f402f5..0eaf21259874 100644 > > > > > > > --- a/drivers/mailbox/mailbox.c > > > > > > > +++ b/drivers/mailbox/mailbox.c > > > > > > > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > > > > > > > unsigned long wait; > > > > > > > int ret; > > > > > > > > > > > > > > + if (irqs_disabled() && chan->mbox->ops->flush) { > > > > > > > + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); > > > > > > > + if (ret < 0) > > > > > > > + tx_tick(chan, ret); > > > > > > > + > > > > > > > + return ret; > > > > > > > + } > > > > > > > > > > > > It seems to me that if mbox_send_message() is called from an atomic > > > > > > context AND tx_block is true, then if 'flush' is not populated this > > > > > > should be an error condition as we do not wish to call > > > > > > wait_for_completion from an atomic context. > > > > > > > > > > > > I understand that there is some debate about adding such flush support, > > > > > > but irrespective of the above change, it seems to me that if the > > > > > > mbox_send_message() can be called from an atomic context (which it > > > > > > appears to), then it should be detecting if someone is trying to do so > > > > > > with 'tx_block' set as this should be an error. > > > > > > > > > > > Layers within kernel space have to trust each other. A badly written > > > > > client can break the consumer in so many ways, we can not catch every > > > > > possibility. > > > > > > > > > > > Furthermore, if the underlying mailbox driver can support sending a > > > > > > message from an atomic context and busy wait until it is done, surely > > > > > > the mailbox framework should provide a means to support this? > > > > > > > > > > > Being able to put the message on bus in atomic context is a feature - > > > > > which we do support. But busy-waiting in a loop is not a feature, and > > > > > we don't want to encourage that. > > > > > > > > I agree that in generally busy-waiting is a bad idea and shouldn't be > > > > encouraged. However, I also think that an exception proves the rule. If > > > > you look at the console drivers in drivers/tty/serial, all of them will > > > > busy loop prior to or after sending a character. This is pretty much > > > > part of the API and as such busy-looping is very much a feature. > > > > > > > > The reason why this is done is because on one hand we have an atomic > > > > context and on the other hand we want to make sure that all characters > > > > actually make it to the console when we print them. > > > > > > > > As an example how this can break, I've taken your suggestion to > > > > implement a producer/consumer mode in the TCU driver where the console > > > > write function will just stash characters into a circular buffer and a > > > > work queue will then use mbox_send_message() to drain the circular > > > > buffer. While this does work on the surface, I was able to concern both > > > > of the issues that I was concerned about: 1) it is easy to overflow the > > > > circular buffer by just dumping enough data at once to the console and > > > > 2) when a crash happens, everything in the kernel stops, including the > > > > consumer workqueue that is supposed to drain the circular buffer and > > > > flush messages to the TCU. The result is that, in my particular case, > > > > the boot log will just stop at a random place in the middle of messages > > > > from much earlier in the boot because the TCU hasn't caught up yet and > > > > there's a lot of characters still in the circular buffer. > > > > > > > > Now, 1) can be mitigated by increasing the circular buffer size. A value > > > > that seems to give reliably good results in 2 << CONFIG_LOG_BUF_SHIFT. > > > > > > > Yes please. > > > > As I explained elsewhere, I actually went and implemented this. But > > given the nature of buffering, this ends up making the TCU completely > > useless as a console because in case of a crash, the system will stop > > working with a large number of characters still stuck in the buffer. > > And that's especially bad in case of a crash because those last > > characters that get stuck in the buffer are the most relevant ones > > because they contain the stack dump. > > > I don't question the utility of TCU. I just wonder if mailbox api > should provide a backdoor for atomic busy-wait in order to support a > sub-optimal hw+fw design. > > > > > I thought that I could also mitigate 2) by busy looping in the TCU driver, > > > > but it turns out that that doesn't work. The reason is that since we are > > > > in atomic context with all interrupts disabled, the mailbox won't ever > > > > consume any new characters, so the read pointer in the circular buffer > > > > won't increment, leaving me with no condition upon which to loop that > > > > would work. > > > > > > > So you want to be able to rely on an emulated console (running on a > > > totally different subsystem) to dump development-phase early-boot > > > logs? At the cost of legitimizing busy looping in atomic context - one > > > random driver messing up the api for ever. Maybe you could have the > > > ring buffer in some shmem and only pass the number of valid characters > > > in it, to the remote? > > > > First of all, this is not about development-phase early-boot messages. > > We're talking about the system console here. This is what everyone will > > want to be using when developing on this device. Sure at some point you > > may end up with a system that works and you can have the console on the > > network or an attached display or whatever, but even then you may still > > want to attach to the console if you ever run into issues where the > > system doesn't come up. > > > > Secondly, I don't understand why you think this is an emulated console. > > > It is emulated/virtual because Linux doesn't put characters on a > physical bus. The data is simply handed forward to a remote entity. Okay, I understand. However, from a kernel point of view there's no difference between the TCU and any physical UART. Once the data is handed to the TCU TX mailbox, it's out of control of the TCU driver. Whether there's a real UART behind that or some remote processor that reads the data isn't really relevant. > > Lastly, I don't understand why you think we're messing up the API here. > > The proposal in this series doesn't even change any of the API, but it > > makes it aware of the state of interrupts internally so that it can do > > the right thing depending on the call stack. The other proposal, in v3, > > is more explicit in that it adds new API to flush the mailbox. The new > > API is completely optional and I even offered to document it as being > > discouraged because it involves busy looping. At the same time it does > > solve a real problem and it doesn't impact any existing mailbox drivers > > nor any of their users (because it is completely opt-in). > > > The 'flush' api is going to have no use other than implement > busy-waits. I am afraid people are simply going to take it easy and > copy the busy-waits from the test/verification codes. > "discouraging" seldom works because the developer comes with the > failproof excuse "the h/w doesn't provide any other way". Frankly I > would like to push back on such designs. I certainly approve of that stance. However, I'd like to reiterate that the TCU design does in fact support "another way". If you look at the mailbox driver implementation (in the drivers/mailbox/tegra-hsp.c driver), you'll see that it does support an interrupt driven mode. After you had reviewed Mikko's earliest proposal that was indeed implementing busy looping exclusively we both set out to properly implement interrupt support. This took quite a while to do because of some gaps in the documentation, but we eventually got it to work properly. And then it was discovered that it was all a waste of effort because the console driver couldn't make use of it anyway. Well, I should say "waste of effort" because I'm still happy that the proper implementation exists and we can use it under the right circumstances. So, at least in this particular case, it is not the hardware or firmware design that's flawed or was taking any shortcuts. It's really just the particular use-case of the console that doesn't allow us to make use of the right implementation, which is why we have to resort to the inferior method of busy-looping. > Anyways, let us add the new 'flush' api. Great, thanks! Thierry
On Mon, Dec 10, 2018 at 3:22 PM Thierry Reding <thierry.reding@gmail.com> wrote: > On Sat, Dec 08, 2018 at 11:39:05AM +0530, Jassi Brar wrote: > > On Fri, Dec 7, 2018 at 5:02 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > Secondly, I don't understand why you think this is an emulated console. > > > > > It is emulated/virtual because Linux doesn't put characters on a > > physical bus. The data is simply handed forward to a remote entity. > > Okay, I understand. However, from a kernel point of view there's no > difference between the TCU and any physical UART. Once the data is > handed to the TCU TX mailbox, it's out of control of the TCU driver. > Whether there's a real UART behind that or some remote processor that > reads the data isn't really relevant. > That way there can be no virtual/emulated device ? > > > > The 'flush' api is going to have no use other than implement > > busy-waits. I am afraid people are simply going to take it easy and > > copy the busy-waits from the test/verification codes. > > "discouraging" seldom works because the developer comes with the > > failproof excuse "the h/w doesn't provide any other way". Frankly I > > would like to push back on such designs. > > I certainly approve of that stance. > > However, I'd like to reiterate that the TCU design does in fact support > "another way". If you look at the mailbox driver implementation (in the > drivers/mailbox/tegra-hsp.c driver), you'll see that it does support an > interrupt driven mode. After you had reviewed Mikko's earliest proposal > that was indeed implementing busy looping exclusively we both set out > to properly implement interrupt support. This took quite a while to do > because of some gaps in the documentation, but we eventually got it to > work properly. And then it was discovered that it was all a waste of > effort because the console driver couldn't make use of it anyway. Well, > I should say "waste of effort" because I'm still happy that the proper > implementation exists and we can use it under the right circumstances. > > So, at least in this particular case, it is not the hardware or firmware > design that's flawed or was taking any shortcuts. It's really just the > particular use-case of the console that doesn't allow us to make use of > the right implementation, which is why we have to resort to the inferior > method of busy-looping. > I am not complaining about the hardware, but the firmware. It is essential we dump logs during early boot but the platform(Linux) doesn't have access to serial port. All the firmware allows is 24bits per transfer!! We could do better. A smarter option could have been Linux simply placing characters in the shmem ring-buffer, while the remote consuming (and then poisoning) the ring buffer upon 'hint' from Linux. -Jassi
On Tue, Dec 11, 2018 at 02:00:48AM +0530, Jassi Brar wrote: > On Mon, Dec 10, 2018 at 3:22 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Sat, Dec 08, 2018 at 11:39:05AM +0530, Jassi Brar wrote: > > > On Fri, Dec 7, 2018 at 5:02 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > > > > > Secondly, I don't understand why you think this is an emulated console. > > > > > > > It is emulated/virtual because Linux doesn't put characters on a > > > physical bus. The data is simply handed forward to a remote entity. > > > > Okay, I understand. However, from a kernel point of view there's no > > difference between the TCU and any physical UART. Once the data is > > handed to the TCU TX mailbox, it's out of control of the TCU driver. > > Whether there's a real UART behind that or some remote processor that > > reads the data isn't really relevant. > > > That way there can be no virtual/emulated device ? Virtual/emulated I interpret as purely implemented in software. That is within the Linux kernel. Once you leave the domain of the kernel it's no longer under the complete control of the kernel. So where exactly it goes doesn't matter, it's pretty much the same as a hardware device from the kernel's perspective. But I guess you could define this differently. > > > > > > The 'flush' api is going to have no use other than implement > > > busy-waits. I am afraid people are simply going to take it easy and > > > copy the busy-waits from the test/verification codes. > > > "discouraging" seldom works because the developer comes with the > > > failproof excuse "the h/w doesn't provide any other way". Frankly I > > > would like to push back on such designs. > > > > I certainly approve of that stance. > > > > However, I'd like to reiterate that the TCU design does in fact support > > "another way". If you look at the mailbox driver implementation (in the > > drivers/mailbox/tegra-hsp.c driver), you'll see that it does support an > > interrupt driven mode. After you had reviewed Mikko's earliest proposal > > that was indeed implementing busy looping exclusively we both set out > > to properly implement interrupt support. This took quite a while to do > > because of some gaps in the documentation, but we eventually got it to > > work properly. And then it was discovered that it was all a waste of > > effort because the console driver couldn't make use of it anyway. Well, > > I should say "waste of effort" because I'm still happy that the proper > > implementation exists and we can use it under the right circumstances. > > > > So, at least in this particular case, it is not the hardware or firmware > > design that's flawed or was taking any shortcuts. It's really just the > > particular use-case of the console that doesn't allow us to make use of > > the right implementation, which is why we have to resort to the inferior > > method of busy-looping. > > > I am not complaining about the hardware, but the firmware. > It is essential we dump logs during early boot but the platform(Linux) > doesn't have access to serial port. All the firmware allows is 24bits > per transfer!! We could do better. Hardware UARTs don't usually have much more FIFO space than that either. > A smarter option could have been Linux simply placing characters in > the shmem ring-buffer, while the remote consuming (and then poisoning) > the ring buffer upon 'hint' from Linux. I don't think that would've been much smarter, especially not in this case. As we discussed earlier, no matter how large you make the ring- buffer you can always run into situations where you overflow it. The ring-buffer implementation is also a lot more complicated and error- prone. Plus there is the fact that in this particular case we actually don't want buffering because the buffer may hide important information in case of a crash. Thierry
On Tue, Dec 11, 2018 at 2:15 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Tue, Dec 11, 2018 at 02:00:48AM +0530, Jassi Brar wrote: > > On Mon, Dec 10, 2018 at 3:22 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > The 'flush' api is going to have no use other than implement > > > > busy-waits. I am afraid people are simply going to take it easy and > > > > copy the busy-waits from the test/verification codes. > > > > "discouraging" seldom works because the developer comes with the > > > > failproof excuse "the h/w doesn't provide any other way". Frankly I > > > > would like to push back on such designs. > > > > > > I certainly approve of that stance. > > > > > > However, I'd like to reiterate that the TCU design does in fact support > > > "another way". If you look at the mailbox driver implementation (in the > > > drivers/mailbox/tegra-hsp.c driver), you'll see that it does support an > > > interrupt driven mode. After you had reviewed Mikko's earliest proposal > > > that was indeed implementing busy looping exclusively we both set out > > > to properly implement interrupt support. This took quite a while to do > > > because of some gaps in the documentation, but we eventually got it to > > > work properly. And then it was discovered that it was all a waste of > > > effort because the console driver couldn't make use of it anyway. Well, > > > I should say "waste of effort" because I'm still happy that the proper > > > implementation exists and we can use it under the right circumstances. > > > > > > So, at least in this particular case, it is not the hardware or firmware > > > design that's flawed or was taking any shortcuts. It's really just the > > > particular use-case of the console that doesn't allow us to make use of > > > the right implementation, which is why we have to resort to the inferior > > > method of busy-looping. > > > > > I am not complaining about the hardware, but the firmware. > > It is essential we dump logs during early boot but the platform(Linux) > > doesn't have access to serial port. All the firmware allows is 24bits > > per transfer!! We could do better. > > Hardware UARTs don't usually have much more FIFO space than that either. > > > A smarter option could have been Linux simply placing characters in > > the shmem ring-buffer, while the remote consuming (and then poisoning) > > the ring buffer upon 'hint' from Linux. > > I don't think that would've been much smarter, especially not in this > case. As we discussed earlier, no matter how large you make the ring- > buffer you can always run into situations where you overflow it. The > ring-buffer implementation is also a lot more complicated and error- > prone. > Please think about it again. The ring buffer becomes the effective h/w fifo. And you don't have to wait at all for the mailbox register to clear .... you could simply overwrite it when Linux puts some data in the ring-buffer (the data written will just be a 'hint' command for remote to go look into the ring-buffer for new data). > Plus there is the fact that in this particular case we actually > don't want buffering because the buffer may hide important information > in case of a crash. > Even if the Linux crashes, whatever data is placed in the ring-buffer will eventually be printed by the still-alive remote. Anyways once we have the flush api, I don't really care how broken your f/w is.
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index 674b35f402f5..0eaf21259874 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) unsigned long wait; int ret; + if (irqs_disabled() && chan->mbox->ops->flush) { + ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout); + if (ret < 0) + tx_tick(chan, ret); + + return ret; + } + if (!chan->cl->tx_tout) /* wait forever */ wait = msecs_to_jiffies(3600000); else diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h index 74deadb42d76..2a07d93f781a 100644 --- a/include/linux/mailbox_controller.h +++ b/include/linux/mailbox_controller.h @@ -24,6 +24,9 @@ struct mbox_chan; * transmission of data is reported by the controller via * mbox_chan_txdone (if it has some TX ACK irq). It must not * sleep. + * @flush: Called when a client requests transmissions to be blocking but + * the context doesn't allow sleeping. Typically the controller + * will implement a busy loop waiting for the data to flush out. * @startup: Called when a client requests the chan. The controller * could ask clients for additional parameters of communication * to be provided via client's chan_data. This call may @@ -46,6 +49,7 @@ struct mbox_chan; */ struct mbox_chan_ops { int (*send_data)(struct mbox_chan *chan, void *data); + int (*flush)(struct mbox_chan *chan, unsigned long timeout); int (*startup)(struct mbox_chan *chan); void (*shutdown)(struct mbox_chan *chan); bool (*last_tx_done)(struct mbox_chan *chan);