Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Sat, 12 Nov 2016, Lu Baolu wrote:
> On 11/11/2016 08:28 PM, Peter Zijlstra wrote:
> > Again, a UART rules. Make a virtual UART in hardware, that'd be totally
> > awesome. This thing, I'm not convinced its worth having.
> 
> This is the initial work. It helps at least in cases where people need
> to dump kernel messages but lacking of a console. It's also a cheap
> way, people don't need to buy any third-party devices.
> 
> With more and more people trying and enhancing it, it will become
> more robust and helpful.

Well, not really. The hardware does not allow you what an UART allows you
to do and never will, no matter how many people are trying to enhance the
software part of it.

Thanks,

tglx

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Sat, 12 Nov 2016, Lu Baolu wrote:
> On 11/11/2016 08:28 PM, Peter Zijlstra wrote:
> > Again, a UART rules. Make a virtual UART in hardware, that'd be totally
> > awesome. This thing, I'm not convinced its worth having.
> 
> This is the initial work. It helps at least in cases where people need
> to dump kernel messages but lacking of a console. It's also a cheap
> way, people don't need to buy any third-party devices.
> 
> With more and more people trying and enhancing it, it will become
> more robust and helpful.

Well, not really. The hardware does not allow you what an UART allows you
to do and never will, no matter how many people are trying to enhance the
software part of it.

Thanks,

tglx

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Peter,

On 11/11/2016 08:28 PM, Peter Zijlstra wrote:
> On Fri, Nov 11, 2016 at 12:33:29PM +0800, Lu Baolu wrote:
>
>> Things become complicated when it comes to USB debug port.
>> But it's still addressable.
>>
>> At this time, we can do it like this.
>>
>> write()
>> {
>>  if (in_nmi() && raw_spin_is_locked())
>>  return;
>>
>>  raw_spinlock_irqsave(, flags);
>>  
>>
> Please use raw_spin_trlock_irqsave() instead, spin_is_locked() is fairly
> icky.

Sure.

>
> Also, there's a bunch of exception contexts that do not set in_nmi().
> That is in_nmi() is really only set for #NM. #MC and #DB and
> others do not set this.

That's worth another fix patch. Let me look into it later.

>
>> This will filter some messages from NMI handler in case that
>> another thread is holding the spinlock. I have no idea about
>> how much chance could a debug user faces this. But it might
>> further be fixed with below enhancement.
>>
>> write()
>> {
>>  if (in_nmi() && raw_spin_is_locked()) {
>>  produce_a_pending_item_in_ring();
>>  return;
>>  }
>>
>>  raw_spinlock_irqsave(, flags);
>>
>>  while (!pending_item_ring_is_empty)
>>  consume_a_pending_item_in_ring();
>>
>>  
>>
>>
>> We can design the pending item ring in a producer-consumer
>> model. It's easy to avoid race between the producer and
>> consumer.
> Problem is that the consumer might never happen, those are the fun most
> bugs.
>
> Not being able to deal with random nested exception context really
> reduces the utility of this thing.
>
> Again, a UART rules. Make a virtual UART in hardware, that'd be totally
> awesome. This thing, I'm not convinced its worth having.

This is the initial work. It helps at least in cases where people need
to dump kernel messages but lacking of a console. It's also a cheap
way, people don't need to buy any third-party devices.

With more and more people trying and enhancing it, it will become
more robust and helpful.

Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Peter,

On 11/11/2016 08:28 PM, Peter Zijlstra wrote:
> On Fri, Nov 11, 2016 at 12:33:29PM +0800, Lu Baolu wrote:
>
>> Things become complicated when it comes to USB debug port.
>> But it's still addressable.
>>
>> At this time, we can do it like this.
>>
>> write()
>> {
>>  if (in_nmi() && raw_spin_is_locked())
>>  return;
>>
>>  raw_spinlock_irqsave(, flags);
>>  
>>
> Please use raw_spin_trlock_irqsave() instead, spin_is_locked() is fairly
> icky.

Sure.

>
> Also, there's a bunch of exception contexts that do not set in_nmi().
> That is in_nmi() is really only set for #NM. #MC and #DB and
> others do not set this.

That's worth another fix patch. Let me look into it later.

>
>> This will filter some messages from NMI handler in case that
>> another thread is holding the spinlock. I have no idea about
>> how much chance could a debug user faces this. But it might
>> further be fixed with below enhancement.
>>
>> write()
>> {
>>  if (in_nmi() && raw_spin_is_locked()) {
>>  produce_a_pending_item_in_ring();
>>  return;
>>  }
>>
>>  raw_spinlock_irqsave(, flags);
>>
>>  while (!pending_item_ring_is_empty)
>>  consume_a_pending_item_in_ring();
>>
>>  
>>
>>
>> We can design the pending item ring in a producer-consumer
>> model. It's easy to avoid race between the producer and
>> consumer.
> Problem is that the consumer might never happen, those are the fun most
> bugs.
>
> Not being able to deal with random nested exception context really
> reduces the utility of this thing.
>
> Again, a UART rules. Make a virtual UART in hardware, that'd be totally
> awesome. This thing, I'm not convinced its worth having.

This is the initial work. It helps at least in cases where people need
to dump kernel messages but lacking of a console. It's also a cheap
way, people don't need to buy any third-party devices.

With more and more people trying and enhancing it, it will become
more robust and helpful.

Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Fri, Nov 11, 2016 at 12:33:29PM +0800, Lu Baolu wrote:

> Things become complicated when it comes to USB debug port.
> But it's still addressable.
> 
> At this time, we can do it like this.
> 
> write()
> {
>   if (in_nmi() && raw_spin_is_locked())
>   return;
> 
>   raw_spinlock_irqsave(, flags);
>   
> 

Please use raw_spin_trlock_irqsave() instead, spin_is_locked() is fairly
icky.

Also, there's a bunch of exception contexts that do not set in_nmi().
That is in_nmi() is really only set for #NM. #MC and #DB and
others do not set this.

> This will filter some messages from NMI handler in case that
> another thread is holding the spinlock. I have no idea about
> how much chance could a debug user faces this. But it might
> further be fixed with below enhancement.
> 
> write()
> {
>   if (in_nmi() && raw_spin_is_locked()) {
>   produce_a_pending_item_in_ring();
>   return;
>   }
> 
>   raw_spinlock_irqsave(, flags);
> 
>   while (!pending_item_ring_is_empty)
>   consume_a_pending_item_in_ring();
> 
>   
> 
> 
> We can design the pending item ring in a producer-consumer
> model. It's easy to avoid race between the producer and
> consumer.

Problem is that the consumer might never happen, those are the fun most
bugs.

Not being able to deal with random nested exception context really
reduces the utility of this thing.

Again, a UART rules. Make a virtual UART in hardware, that'd be totally
awesome. This thing, I'm not convinced its worth having.

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Fri, Nov 11, 2016 at 12:33:29PM +0800, Lu Baolu wrote:

> Things become complicated when it comes to USB debug port.
> But it's still addressable.
> 
> At this time, we can do it like this.
> 
> write()
> {
>   if (in_nmi() && raw_spin_is_locked())
>   return;
> 
>   raw_spinlock_irqsave(, flags);
>   
> 

Please use raw_spin_trlock_irqsave() instead, spin_is_locked() is fairly
icky.

Also, there's a bunch of exception contexts that do not set in_nmi().
That is in_nmi() is really only set for #NM. #MC and #DB and
others do not set this.

> This will filter some messages from NMI handler in case that
> another thread is holding the spinlock. I have no idea about
> how much chance could a debug user faces this. But it might
> further be fixed with below enhancement.
> 
> write()
> {
>   if (in_nmi() && raw_spin_is_locked()) {
>   produce_a_pending_item_in_ring();
>   return;
>   }
> 
>   raw_spinlock_irqsave(, flags);
> 
>   while (!pending_item_ring_is_empty)
>   consume_a_pending_item_in_ring();
> 
>   
> 
> 
> We can design the pending item ring in a producer-consumer
> model. It's easy to avoid race between the producer and
> consumer.

Problem is that the consumer might never happen, those are the fun most
bugs.

Not being able to deal with random nested exception context really
reduces the utility of this thing.

Again, a UART rules. Make a virtual UART in hardware, that'd be totally
awesome. This thing, I'm not convinced its worth having.

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Peter,

On 11/10/2016 07:44 PM, Peter Zijlstra wrote:
> On Thu, Nov 10, 2016 at 09:56:41AM +0100, Thomas Gleixner wrote:
>> On Thu, 10 Nov 2016, Lu Baolu wrote:
>>> This seems to be a common issue for all early printk drivers.
>> No. The other early printk drivers like serial do not have that problem as
>> they simply do:
>>
>>while (*buf) {
>>   while (inb(UART) & TX_BUSY)
>>   cpu_relax();
>>   outb(*buf++, UART);
>>}
> Right, which is why actual UARTs rule. If only laptops still had pinouts
> for them life would be so much better.
>
> Ideally the USB debug port would be a virtual UART and its interface as
> simple and robust.
>
>> The wait for the UART to become ready is independent of the context as it
>> solely depends on the hardware.
>>
>> As a result you can see the output from irq/nmi intermingled with the one
>> from thread context, but that's the only problem they have.
>>
>> The only thing you can do to make this work is to prevent printing in NMI
>> context:
>>
>> write()
>> {
>>  if (in_nmi())
>>  return;
>>  
>>  raw_spinlock_irqsave(, flags);
>>  
>>
>> That fully serializes the writes and just ignores NMI context printks. Not
>> optimal, but I fear that's all you can do.
> I would also suggest telling the hardware people they have designed
> something near the brink of useless. If you cannot do random exception
> context debugging (#DB, #NMI, #MCE etc..) then there's a whole host of
> problems that simply cannot be debugged.
>
> Also note that kdb runs from NMI context, so you'll not be able to
> support that either.
>

Things become complicated when it comes to USB debug port.
But it's still addressable.

At this time, we can do it like this.

write()
{
if (in_nmi() && raw_spin_is_locked())
return;

raw_spinlock_irqsave(, flags);



This will filter some messages from NMI handler in case that
another thread is holding the spinlock. I have no idea about
how much chance could a debug user faces this. But it might
further be fixed with below enhancement.

write()
{
if (in_nmi() && raw_spin_is_locked()) {
produce_a_pending_item_in_ring();
return;
}

raw_spinlock_irqsave(, flags);

while (!pending_item_ring_is_empty)
consume_a_pending_item_in_ring();




We can design the pending item ring in a producer-consumer
model. It's easy to avoid race between the producer and
consumer.

Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Peter,

On 11/10/2016 07:44 PM, Peter Zijlstra wrote:
> On Thu, Nov 10, 2016 at 09:56:41AM +0100, Thomas Gleixner wrote:
>> On Thu, 10 Nov 2016, Lu Baolu wrote:
>>> This seems to be a common issue for all early printk drivers.
>> No. The other early printk drivers like serial do not have that problem as
>> they simply do:
>>
>>while (*buf) {
>>   while (inb(UART) & TX_BUSY)
>>   cpu_relax();
>>   outb(*buf++, UART);
>>}
> Right, which is why actual UARTs rule. If only laptops still had pinouts
> for them life would be so much better.
>
> Ideally the USB debug port would be a virtual UART and its interface as
> simple and robust.
>
>> The wait for the UART to become ready is independent of the context as it
>> solely depends on the hardware.
>>
>> As a result you can see the output from irq/nmi intermingled with the one
>> from thread context, but that's the only problem they have.
>>
>> The only thing you can do to make this work is to prevent printing in NMI
>> context:
>>
>> write()
>> {
>>  if (in_nmi())
>>  return;
>>  
>>  raw_spinlock_irqsave(, flags);
>>  
>>
>> That fully serializes the writes and just ignores NMI context printks. Not
>> optimal, but I fear that's all you can do.
> I would also suggest telling the hardware people they have designed
> something near the brink of useless. If you cannot do random exception
> context debugging (#DB, #NMI, #MCE etc..) then there's a whole host of
> problems that simply cannot be debugged.
>
> Also note that kdb runs from NMI context, so you'll not be able to
> support that either.
>

Things become complicated when it comes to USB debug port.
But it's still addressable.

At this time, we can do it like this.

write()
{
if (in_nmi() && raw_spin_is_locked())
return;

raw_spinlock_irqsave(, flags);



This will filter some messages from NMI handler in case that
another thread is holding the spinlock. I have no idea about
how much chance could a debug user faces this. But it might
further be fixed with below enhancement.

write()
{
if (in_nmi() && raw_spin_is_locked()) {
produce_a_pending_item_in_ring();
return;
}

raw_spinlock_irqsave(, flags);

while (!pending_item_ring_is_empty)
consume_a_pending_item_in_ring();




We can design the pending item ring in a producer-consumer
model. It's easy to avoid race between the producer and
consumer.

Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 11/11/2016 10:24 AM, Lu Baolu wrote:
>> The only thing you can do to make this work is to prevent printing in NMI
>> > context:
>> >
>> > write()
>> > {
>> >if (in_nmi())
>> >return;
>> >
>> >raw_spinlock_irqsave(, flags);
>> >
>> >
>> > That fully serializes the writes and just ignores NMI context printks. Not
>> > optimal, but I fear that's all you can do.
> Yes. But I want to add a bit more.
>
> write()
> {
>   if (in_nmi() && raw_spin_is_locked()) {
>   trace("... ...");
>   return;
>   }
>
>   raw_spinlock_irqsave(, flags);
>   

Or...?

write()
{
if (in_nmi() && raw_spin_is_locked()) {
save_nmi_message_in_local_buf();
set_nmi_message_pending_flag();
return;
}

if (nmi_message_pending_flag_is_set()) {
write_nmi_message();
clear_nmi_message_pending_flag();
}

raw_spinlock_irqsave(, flags);



Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 11/11/2016 10:24 AM, Lu Baolu wrote:
>> The only thing you can do to make this work is to prevent printing in NMI
>> > context:
>> >
>> > write()
>> > {
>> >if (in_nmi())
>> >return;
>> >
>> >raw_spinlock_irqsave(, flags);
>> >
>> >
>> > That fully serializes the writes and just ignores NMI context printks. Not
>> > optimal, but I fear that's all you can do.
> Yes. But I want to add a bit more.
>
> write()
> {
>   if (in_nmi() && raw_spin_is_locked()) {
>   trace("... ...");
>   return;
>   }
>
>   raw_spinlock_irqsave(, flags);
>   

Or...?

write()
{
if (in_nmi() && raw_spin_is_locked()) {
save_nmi_message_in_local_buf();
set_nmi_message_pending_flag();
return;
}

if (nmi_message_pending_flag_is_set()) {
write_nmi_message();
clear_nmi_message_pending_flag();
}

raw_spinlock_irqsave(, flags);



Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 11/10/2016 04:56 PM, Thomas Gleixner wrote:
> On Thu, 10 Nov 2016, Lu Baolu wrote:
>> On 11/09/2016 05:37 PM, Thomas Gleixner wrote:
>>> On Tue, 1 Nov 2016, Lu Baolu wrote:
 +static void early_xdbc_write(struct console *con, const char *str, u32 n)
 +{
 +  int chunk, ret;
 +  static char buf[XDBC_MAX_PACKET];
 +  int use_cr = 0;
 +
 +  if (!xdbc.xdbc_reg)
 +  return;
 +  memset(buf, 0, XDBC_MAX_PACKET);
>>> How is that dealing with reentrancy?
>>>
>>> early_printk() does not protect against it. Peter has a patch to prevent
>>> concurrent access from different cpus, but it cannot and will never prevent
>>> reentrancy on the same cpu (interrupt, nmi).
>> I can use a spinlock_irq to protect reentrancy of interrupt on the same
>> cpu. But I have no idea about the nmi one.
> spinlock wont work due to NMIs.

Yes, of course.

>
>> This seems to be a common issue for all early printk drivers.
> No. The other early printk drivers like serial do not have that problem as
> they simply do:
>
>while (*buf) {
>   while (inb(UART) & TX_BUSY)
>cpu_relax();
>   outb(*buf++, UART);
>}
>
> The wait for the UART to become ready is independent of the context as it
> solely depends on the hardware.
>
> As a result you can see the output from irq/nmi intermingled with the one
> from thread context, but that's the only problem they have.

Yes, you are right.

>
> The only thing you can do to make this work is to prevent printing in NMI
> context:
>
> write()
> {
>   if (in_nmi())
>   return;
>   
>   raw_spinlock_irqsave(, flags);
>   
>
> That fully serializes the writes and just ignores NMI context printks. Not
> optimal, but I fear that's all you can do.

Yes. But I want to add a bit more.

write()
{
if (in_nmi() && raw_spin_is_locked()) {
trace("... ...");
return;
}

raw_spinlock_irqsave(, flags);



Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 11/10/2016 04:56 PM, Thomas Gleixner wrote:
> On Thu, 10 Nov 2016, Lu Baolu wrote:
>> On 11/09/2016 05:37 PM, Thomas Gleixner wrote:
>>> On Tue, 1 Nov 2016, Lu Baolu wrote:
 +static void early_xdbc_write(struct console *con, const char *str, u32 n)
 +{
 +  int chunk, ret;
 +  static char buf[XDBC_MAX_PACKET];
 +  int use_cr = 0;
 +
 +  if (!xdbc.xdbc_reg)
 +  return;
 +  memset(buf, 0, XDBC_MAX_PACKET);
>>> How is that dealing with reentrancy?
>>>
>>> early_printk() does not protect against it. Peter has a patch to prevent
>>> concurrent access from different cpus, but it cannot and will never prevent
>>> reentrancy on the same cpu (interrupt, nmi).
>> I can use a spinlock_irq to protect reentrancy of interrupt on the same
>> cpu. But I have no idea about the nmi one.
> spinlock wont work due to NMIs.

Yes, of course.

>
>> This seems to be a common issue for all early printk drivers.
> No. The other early printk drivers like serial do not have that problem as
> they simply do:
>
>while (*buf) {
>   while (inb(UART) & TX_BUSY)
>cpu_relax();
>   outb(*buf++, UART);
>}
>
> The wait for the UART to become ready is independent of the context as it
> solely depends on the hardware.
>
> As a result you can see the output from irq/nmi intermingled with the one
> from thread context, but that's the only problem they have.

Yes, you are right.

>
> The only thing you can do to make this work is to prevent printing in NMI
> context:
>
> write()
> {
>   if (in_nmi())
>   return;
>   
>   raw_spinlock_irqsave(, flags);
>   
>
> That fully serializes the writes and just ignores NMI context printks. Not
> optimal, but I fear that's all you can do.

Yes. But I want to add a bit more.

write()
{
if (in_nmi() && raw_spin_is_locked()) {
trace("... ...");
return;
}

raw_spinlock_irqsave(, flags);



Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Thu, Nov 10, 2016 at 09:56:41AM +0100, Thomas Gleixner wrote:
> On Thu, 10 Nov 2016, Lu Baolu wrote:

> > This seems to be a common issue for all early printk drivers.
> 
> No. The other early printk drivers like serial do not have that problem as
> they simply do:
> 
>while (*buf) {
>   while (inb(UART) & TX_BUSY)
>cpu_relax();
>   outb(*buf++, UART);
>}

Right, which is why actual UARTs rule. If only laptops still had pinouts
for them life would be so much better.

Ideally the USB debug port would be a virtual UART and its interface as
simple and robust.

> The wait for the UART to become ready is independent of the context as it
> solely depends on the hardware.
> 
> As a result you can see the output from irq/nmi intermingled with the one
> from thread context, but that's the only problem they have.
> 
> The only thing you can do to make this work is to prevent printing in NMI
> context:
> 
> write()
> {
>   if (in_nmi())
>   return;
>   
>   raw_spinlock_irqsave(, flags);
>   
> 
> That fully serializes the writes and just ignores NMI context printks. Not
> optimal, but I fear that's all you can do.

I would also suggest telling the hardware people they have designed
something near the brink of useless. If you cannot do random exception
context debugging (#DB, #NMI, #MCE etc..) then there's a whole host of
problems that simply cannot be debugged.

Also note that kdb runs from NMI context, so you'll not be able to
support that either.

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Thu, Nov 10, 2016 at 09:56:41AM +0100, Thomas Gleixner wrote:
> On Thu, 10 Nov 2016, Lu Baolu wrote:

> > This seems to be a common issue for all early printk drivers.
> 
> No. The other early printk drivers like serial do not have that problem as
> they simply do:
> 
>while (*buf) {
>   while (inb(UART) & TX_BUSY)
>cpu_relax();
>   outb(*buf++, UART);
>}

Right, which is why actual UARTs rule. If only laptops still had pinouts
for them life would be so much better.

Ideally the USB debug port would be a virtual UART and its interface as
simple and robust.

> The wait for the UART to become ready is independent of the context as it
> solely depends on the hardware.
> 
> As a result you can see the output from irq/nmi intermingled with the one
> from thread context, but that's the only problem they have.
> 
> The only thing you can do to make this work is to prevent printing in NMI
> context:
> 
> write()
> {
>   if (in_nmi())
>   return;
>   
>   raw_spinlock_irqsave(, flags);
>   
> 
> That fully serializes the writes and just ignores NMI context printks. Not
> optimal, but I fear that's all you can do.

I would also suggest telling the hardware people they have designed
something near the brink of useless. If you cannot do random exception
context debugging (#DB, #NMI, #MCE etc..) then there's a whole host of
problems that simply cannot be debugged.

Also note that kdb runs from NMI context, so you'll not be able to
support that either.

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Thu, 10 Nov 2016, Lu Baolu wrote:
> On 11/09/2016 05:37 PM, Thomas Gleixner wrote:
> > On Tue, 1 Nov 2016, Lu Baolu wrote:
> >> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
> >> +{
> >> +  int chunk, ret;
> >> +  static char buf[XDBC_MAX_PACKET];
> >> +  int use_cr = 0;
> >> +
> >> +  if (!xdbc.xdbc_reg)
> >> +  return;
> >> +  memset(buf, 0, XDBC_MAX_PACKET);
> > How is that dealing with reentrancy?
> >
> > early_printk() does not protect against it. Peter has a patch to prevent
> > concurrent access from different cpus, but it cannot and will never prevent
> > reentrancy on the same cpu (interrupt, nmi).
> 
> I can use a spinlock_irq to protect reentrancy of interrupt on the same
> cpu. But I have no idea about the nmi one.

spinlock wont work due to NMIs.

> This seems to be a common issue for all early printk drivers.

No. The other early printk drivers like serial do not have that problem as
they simply do:

   while (*buf) {
  while (inb(UART) & TX_BUSY)
 cpu_relax();
  outb(*buf++, UART);
   }

The wait for the UART to become ready is independent of the context as it
solely depends on the hardware.

As a result you can see the output from irq/nmi intermingled with the one
from thread context, but that's the only problem they have.

The only thing you can do to make this work is to prevent printing in NMI
context:

write()
{
if (in_nmi())
return;

raw_spinlock_irqsave(, flags);


That fully serializes the writes and just ignores NMI context printks. Not
optimal, but I fear that's all you can do.

Thanks,

tglx

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Thu, 10 Nov 2016, Lu Baolu wrote:
> On 11/09/2016 05:37 PM, Thomas Gleixner wrote:
> > On Tue, 1 Nov 2016, Lu Baolu wrote:
> >> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
> >> +{
> >> +  int chunk, ret;
> >> +  static char buf[XDBC_MAX_PACKET];
> >> +  int use_cr = 0;
> >> +
> >> +  if (!xdbc.xdbc_reg)
> >> +  return;
> >> +  memset(buf, 0, XDBC_MAX_PACKET);
> > How is that dealing with reentrancy?
> >
> > early_printk() does not protect against it. Peter has a patch to prevent
> > concurrent access from different cpus, but it cannot and will never prevent
> > reentrancy on the same cpu (interrupt, nmi).
> 
> I can use a spinlock_irq to protect reentrancy of interrupt on the same
> cpu. But I have no idea about the nmi one.

spinlock wont work due to NMIs.

> This seems to be a common issue for all early printk drivers.

No. The other early printk drivers like serial do not have that problem as
they simply do:

   while (*buf) {
  while (inb(UART) & TX_BUSY)
 cpu_relax();
  outb(*buf++, UART);
   }

The wait for the UART to become ready is independent of the context as it
solely depends on the hardware.

As a result you can see the output from irq/nmi intermingled with the one
from thread context, but that's the only problem they have.

The only thing you can do to make this work is to prevent printing in NMI
context:

write()
{
if (in_nmi())
return;

raw_spinlock_irqsave(, flags);


That fully serializes the writes and just ignores NMI context printks. Not
optimal, but I fear that's all you can do.

Thanks,

tglx

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 11/09/2016 05:37 PM, Thomas Gleixner wrote:
> On Tue, 1 Nov 2016, Lu Baolu wrote:
>> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
>> +{
>> +int chunk, ret;
>> +static char buf[XDBC_MAX_PACKET];
>> +int use_cr = 0;
>> +
>> +if (!xdbc.xdbc_reg)
>> +return;
>> +memset(buf, 0, XDBC_MAX_PACKET);
> How is that dealing with reentrancy?
>
> early_printk() does not protect against it. Peter has a patch to prevent
> concurrent access from different cpus, but it cannot and will never prevent
> reentrancy on the same cpu (interrupt, nmi).

I can use a spinlock_irq to protect reentrancy of interrupt on the same
cpu. But I have no idea about the nmi one. This seems to be a common
issue for all early printk drivers.

Peter, any idea?

Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 11/09/2016 05:37 PM, Thomas Gleixner wrote:
> On Tue, 1 Nov 2016, Lu Baolu wrote:
>> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
>> +{
>> +int chunk, ret;
>> +static char buf[XDBC_MAX_PACKET];
>> +int use_cr = 0;
>> +
>> +if (!xdbc.xdbc_reg)
>> +return;
>> +memset(buf, 0, XDBC_MAX_PACKET);
> How is that dealing with reentrancy?
>
> early_printk() does not protect against it. Peter has a patch to prevent
> concurrent access from different cpus, but it cannot and will never prevent
> reentrancy on the same cpu (interrupt, nmi).

I can use a spinlock_irq to protect reentrancy of interrupt on the same
cpu. But I have no idea about the nmi one. This seems to be a common
issue for all early printk drivers.

Peter, any idea?

Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 11/09/2016 05:23 PM, Thomas Gleixner wrote:
> On Tue, 1 Nov 2016, Lu Baolu wrote:
>> +static int __init xdbc_init(void)
>> +{
> ...
>> +base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
>> +if (!base) {
>> +xdbc_trace("failed to remap the io address\n");
>> +ret = -ENOMEM;
>> +goto free_and_quit;
>> +}
>> +
>> +early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
>> +xdbc_trace("early mapped IO address released\n");
>> +
>> +xdbc.xhci_base = base;
>> +offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
>> +xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
> This is broken. What prevents that 
>
>  - a printk is in progress on another cpu?
>
>  - a printk happens between the unmap and storing the new base ?
>
> Nothing AFAICT. So this needs to be done in a safe way. And just making it
>
>   oldbase = xdbc.xhci_base;
>   base = ioremap();
>   xdbc.xhci_base = base;
>   early_iounmap(oldbase);
>
> does not work either because the compiler can rightfully cache
> xdbc.xhci_base in the write related functions. The same issue with
> xdbc.xdbc_reg.

If there isn't a good solution, I will remove the ioremap code and let
it use the early mapped one.

Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 11/09/2016 05:23 PM, Thomas Gleixner wrote:
> On Tue, 1 Nov 2016, Lu Baolu wrote:
>> +static int __init xdbc_init(void)
>> +{
> ...
>> +base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
>> +if (!base) {
>> +xdbc_trace("failed to remap the io address\n");
>> +ret = -ENOMEM;
>> +goto free_and_quit;
>> +}
>> +
>> +early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
>> +xdbc_trace("early mapped IO address released\n");
>> +
>> +xdbc.xhci_base = base;
>> +offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
>> +xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
> This is broken. What prevents that 
>
>  - a printk is in progress on another cpu?
>
>  - a printk happens between the unmap and storing the new base ?
>
> Nothing AFAICT. So this needs to be done in a safe way. And just making it
>
>   oldbase = xdbc.xhci_base;
>   base = ioremap();
>   xdbc.xhci_base = base;
>   early_iounmap(oldbase);
>
> does not work either because the compiler can rightfully cache
> xdbc.xhci_base in the write related functions. The same issue with
> xdbc.xdbc_reg.

If there isn't a good solution, I will remove the ioremap code and let
it use the early mapped one.

Best regards,
Lu Baolu

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Tue, 1 Nov 2016, Lu Baolu wrote:
> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
> +{
> + int chunk, ret;
> + static char buf[XDBC_MAX_PACKET];
> + int use_cr = 0;
> +
> + if (!xdbc.xdbc_reg)
> + return;
> + memset(buf, 0, XDBC_MAX_PACKET);

How is that dealing with reentrancy?

early_printk() does not protect against it. Peter has a patch to prevent
concurrent access from different cpus, but it cannot and will never prevent
reentrancy on the same cpu (interrupt, nmi).

Thanks,

tglx

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Tue, 1 Nov 2016, Lu Baolu wrote:
> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
> +{
> + int chunk, ret;
> + static char buf[XDBC_MAX_PACKET];
> + int use_cr = 0;
> +
> + if (!xdbc.xdbc_reg)
> + return;
> + memset(buf, 0, XDBC_MAX_PACKET);

How is that dealing with reentrancy?

early_printk() does not protect against it. Peter has a patch to prevent
concurrent access from different cpus, but it cannot and will never prevent
reentrancy on the same cpu (interrupt, nmi).

Thanks,

tglx

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Tue, 1 Nov 2016, Lu Baolu wrote:
> +static int __init xdbc_init(void)
> +{
...
> + base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
> + if (!base) {
> + xdbc_trace("failed to remap the io address\n");
> + ret = -ENOMEM;
> + goto free_and_quit;
> + }
> +
> + early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
> + xdbc_trace("early mapped IO address released\n");
> +
> + xdbc.xhci_base = base;
> + offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
> + xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);

This is broken. What prevents that 

 - a printk is in progress on another cpu?

 - a printk happens between the unmap and storing the new base ?

Nothing AFAICT. So this needs to be done in a safe way. And just making it

oldbase = xdbc.xhci_base;
base = ioremap();
xdbc.xhci_base = base;
early_iounmap(oldbase);

does not work either because the compiler can rightfully cache
xdbc.xhci_base in the write related functions. The same issue with
xdbc.xdbc_reg.

Thanks,

tglx

Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Tue, 1 Nov 2016, Lu Baolu wrote:
> +static int __init xdbc_init(void)
> +{
...
> + base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
> + if (!base) {
> + xdbc_trace("failed to remap the io address\n");
> + ret = -ENOMEM;
> + goto free_and_quit;
> + }
> +
> + early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
> + xdbc_trace("early mapped IO address released\n");
> +
> + xdbc.xhci_base = base;
> + offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
> + xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);

This is broken. What prevents that 

 - a printk is in progress on another cpu?

 - a printk happens between the unmap and storing the new base ?

Nothing AFAICT. So this needs to be done in a safe way. And just making it

oldbase = xdbc.xhci_base;
base = ioremap();
xdbc.xhci_base = base;
early_iounmap(oldbase);

does not work either because the compiler can rightfully cache
xdbc.xhci_base in the write related functions. The same issue with
xdbc.xdbc_reg.

Thanks,

tglx

[PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

xHCI debug capability (DbC) is an optional but standalone
functionality provided by an xHCI host controller. Software
learns this capability by walking through the extended
capability list of the host. xHCI specification describes
DbC in section 7.6.

This patch introduces the code to probe and initialize the
debug capability hardware during early boot. With hardware
initialized, the debug target (system on which this code is
running) will present a debug device through the debug port
(normally the first USB3 port). The debug device is fully
compliant with the USB framework and provides the equivalent
of a very high performance (USB3) full-duplex serial link
between the debug host and target. The DbC functionality is
independent of xHCI host. There isn't any precondition from
xHCI host side for DbC to work.

This patch also includes bulk out and bulk in interfaces.
These interfaces could be used to implement early printk
bootconsole or hook to various system debuggers.

This code is designed to be only used for kernel debugging
when machine crashes very early before the console code is
initialized. For normal operation it is not recommended.

Cc: Mathias Nyman 
Signed-off-by: Lu Baolu 
---
 arch/x86/Kconfig.debug|   14 +
 drivers/usb/Kconfig   |3 +
 drivers/usb/Makefile  |2 +-
 drivers/usb/early/Makefile|1 +
 drivers/usb/early/xhci-dbc.c  | 1050 +
 drivers/usb/early/xhci-dbc.h  |  202 
 include/linux/usb/xhci-dbgp.h |   22 +
 7 files changed, 1293 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/early/xhci-dbc.c
 create mode 100644 drivers/usb/early/xhci-dbc.h
 create mode 100644 include/linux/usb/xhci-dbgp.h

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 67eec55..13e85b7 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -29,6 +29,7 @@ config EARLY_PRINTK
 config EARLY_PRINTK_DBGP
bool "Early printk via EHCI debug port"
depends on EARLY_PRINTK && PCI
+   select USB_EARLY_PRINTK
---help---
  Write kernel log output directly into the EHCI debug port.
 
@@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
  This is useful for kernel debugging when your machine crashes very
  early before the console code is initialized.
 
+config EARLY_PRINTK_XDBC
+   bool "Early printk via xHCI debug port"
+   depends on EARLY_PRINTK && PCI
+   select USB_EARLY_PRINTK
+   ---help---
+ Write kernel log output directly into the xHCI debug port.
+
+ This is useful for kernel debugging when your machine crashes very
+ early before the console code is initialized. For normal operation
+ it is not recommended because it looks ugly and doesn't cooperate
+ with klogd/syslogd or the X server. You should normally N here,
+ unless you want to debug such a crash.
+
 config X86_PTDUMP_CORE
def_bool n
 
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index fbe493d..9313fff 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO
 config USB_EHCI_BIG_ENDIAN_DESC
bool
 
+config USB_EARLY_PRINTK
+   bool
+
 menuconfig USB_SUPPORT
bool "USB support"
depends on HAS_IOMEM
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 7791af6..0c37838 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -49,7 +49,7 @@ obj-$(CONFIG_USB_MICROTEK)+= image/
 obj-$(CONFIG_USB_SERIAL)   += serial/
 
 obj-$(CONFIG_USB)  += misc/
-obj-$(CONFIG_EARLY_PRINTK_DBGP)+= early/
+obj-$(CONFIG_USB_EARLY_PRINTK) += early/
 
 obj-$(CONFIG_USB_ATM)  += atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)   += atm/
diff --git a/drivers/usb/early/Makefile b/drivers/usb/early/Makefile
index 24bbe51..2db5906 100644
--- a/drivers/usb/early/Makefile
+++ b/drivers/usb/early/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o
+obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
new file mode 100644
index 000..728a1a0
--- /dev/null
+++ b/drivers/usb/early/xhci-dbc.c
@@ -0,0 +1,1050 @@
+/**
+ * xhci-dbc.c - xHCI debug capability early driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt)KBUILD_MODNAME ":%s: " fmt, __func__
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "../host/xhci.h"
+#include "xhci-dbc.h"
+
+static struct xdbc_state xdbc;
+static int early_console_keep;
+
+#ifdef XDBC_TRACE

[PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

xHCI debug capability (DbC) is an optional but standalone
functionality provided by an xHCI host controller. Software
learns this capability by walking through the extended
capability list of the host. xHCI specification describes
DbC in section 7.6.

This patch introduces the code to probe and initialize the
debug capability hardware during early boot. With hardware
initialized, the debug target (system on which this code is
running) will present a debug device through the debug port
(normally the first USB3 port). The debug device is fully
compliant with the USB framework and provides the equivalent
of a very high performance (USB3) full-duplex serial link
between the debug host and target. The DbC functionality is
independent of xHCI host. There isn't any precondition from
xHCI host side for DbC to work.

This patch also includes bulk out and bulk in interfaces.
These interfaces could be used to implement early printk
bootconsole or hook to various system debuggers.

This code is designed to be only used for kernel debugging
when machine crashes very early before the console code is
initialized. For normal operation it is not recommended.

Cc: Mathias Nyman 
Signed-off-by: Lu Baolu 
---
 arch/x86/Kconfig.debug|   14 +
 drivers/usb/Kconfig   |3 +
 drivers/usb/Makefile  |2 +-
 drivers/usb/early/Makefile|1 +
 drivers/usb/early/xhci-dbc.c  | 1050 +
 drivers/usb/early/xhci-dbc.h  |  202 
 include/linux/usb/xhci-dbgp.h |   22 +
 7 files changed, 1293 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/early/xhci-dbc.c
 create mode 100644 drivers/usb/early/xhci-dbc.h
 create mode 100644 include/linux/usb/xhci-dbgp.h

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 67eec55..13e85b7 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -29,6 +29,7 @@ config EARLY_PRINTK
 config EARLY_PRINTK_DBGP
bool "Early printk via EHCI debug port"
depends on EARLY_PRINTK && PCI
+   select USB_EARLY_PRINTK
---help---
  Write kernel log output directly into the EHCI debug port.
 
@@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
  This is useful for kernel debugging when your machine crashes very
  early before the console code is initialized.
 
+config EARLY_PRINTK_XDBC
+   bool "Early printk via xHCI debug port"
+   depends on EARLY_PRINTK && PCI
+   select USB_EARLY_PRINTK
+   ---help---
+ Write kernel log output directly into the xHCI debug port.
+
+ This is useful for kernel debugging when your machine crashes very
+ early before the console code is initialized. For normal operation
+ it is not recommended because it looks ugly and doesn't cooperate
+ with klogd/syslogd or the X server. You should normally N here,
+ unless you want to debug such a crash.
+
 config X86_PTDUMP_CORE
def_bool n
 
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index fbe493d..9313fff 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO
 config USB_EHCI_BIG_ENDIAN_DESC
bool
 
+config USB_EARLY_PRINTK
+   bool
+
 menuconfig USB_SUPPORT
bool "USB support"
depends on HAS_IOMEM
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 7791af6..0c37838 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -49,7 +49,7 @@ obj-$(CONFIG_USB_MICROTEK)+= image/
 obj-$(CONFIG_USB_SERIAL)   += serial/
 
 obj-$(CONFIG_USB)  += misc/
-obj-$(CONFIG_EARLY_PRINTK_DBGP)+= early/
+obj-$(CONFIG_USB_EARLY_PRINTK) += early/
 
 obj-$(CONFIG_USB_ATM)  += atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)   += atm/
diff --git a/drivers/usb/early/Makefile b/drivers/usb/early/Makefile
index 24bbe51..2db5906 100644
--- a/drivers/usb/early/Makefile
+++ b/drivers/usb/early/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o
+obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
new file mode 100644
index 000..728a1a0
--- /dev/null
+++ b/drivers/usb/early/xhci-dbc.c
@@ -0,0 +1,1050 @@
+/**
+ * xhci-dbc.c - xHCI debug capability early driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt)KBUILD_MODNAME ":%s: " fmt, __func__
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "../host/xhci.h"
+#include "xhci-dbc.h"
+
+static struct xdbc_state xdbc;
+static int early_console_keep;
+
+#ifdef XDBC_TRACE
+#definexdbc_trace  trace_printk
+#else
+static inline void