* [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
@ 2018-09-28 9:40 Feng Tang
2018-09-29 16:34 ` Borislav Petkov
0 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2018-09-28 9:40 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H Peter Anvin, Borislav Petkov,
Peter Zijlstra, Stuart R . Anderson, alan, x86, linux-kernel
Cc: Feng Tang
"pciserial" earlyprintk helps much on many modern x86 platforms,
but unfortunately there are still some platforms whose PCI UART
devices have wrong PCI class code, which will be blocked by current
class code check.
Add a option "force" so that developer could still use a UART device
even it has wrong class code, with format "",B:D.F,force,baud". And
the original format ",B:D.F,baud" is kept unchanged.
Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Feng Tang <feng.tang@intel.com>
---
arch/x86/kernel/early_printk.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 5e801c8..35d0f66 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -213,8 +213,10 @@ static unsigned int mem32_serial_in(unsigned long addr, int offset)
* early_pci_serial_init()
*
* This function is invoked when the early_printk param starts with "pciserial"
- * The rest of the param should be ",B:D.F,baud" where B, D & F describe the
- * location of a PCI device that must be a UART device.
+ * The rest of the param should be ",B:D.F,baud" or ",B:D.F,force,baud", where
+ * B, D & F describe the location of a PCI device that must be a UART device,
+ * "force" is optional and means insisting using a UART device with a wrong
+ * pci class code.
*/
static __init void early_pci_serial_init(char *s)
{
@@ -224,6 +226,7 @@ static __init void early_pci_serial_init(char *s)
u32 classcode, bar0;
u16 cmdreg;
char *e;
+ int force = 0;
/*
@@ -252,6 +255,15 @@ static __init void early_pci_serial_init(char *s)
if (*s == ',')
s++;
+ /* User may insist to use a UART device with wrong class code */
+ if (!strncmp(s, "force", 5)) {
+ force = 1;
+ s += 5;
+
+ if (*s == ',')
+ s++;
+ }
+
/*
* Second, find the device from the BDF
*/
@@ -262,10 +274,12 @@ static __init void early_pci_serial_init(char *s)
/*
* Verify it is a UART type device
*/
- if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
- (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
- (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
- return;
+ if (!force) {
+ if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
+ (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
+ (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
+ return;
+ }
/*
* Determine if it is IO or memory mapped
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-09-28 9:40 [PATCH v2] x86/earlyprintk: Add a force option for pciserial device Feng Tang
@ 2018-09-29 16:34 ` Borislav Petkov
2018-09-30 13:16 ` Feng Tang
0 siblings, 1 reply; 14+ messages in thread
From: Borislav Petkov @ 2018-09-29 16:34 UTC (permalink / raw)
To: Feng Tang
Cc: Thomas Gleixner, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
On Fri, Sep 28, 2018 at 05:40:08PM +0800, Feng Tang wrote:
> "pciserial" earlyprintk helps much on many modern x86 platforms,
> but unfortunately there are still some platforms whose PCI UART
> devices have wrong PCI class code, which will be blocked by current
> class code check.
>
> Add a option "force" so that developer could still use a UART device
> even it has wrong class code, with format "",B:D.F,force,baud". And
This new parameter and its meaning needs to be documented where
pciserial is documented.
> the original format ",B:D.F,baud" is kept unchanged.
>
> Suggested-by: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
> arch/x86/kernel/early_printk.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index 5e801c8..35d0f66 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -213,8 +213,10 @@ static unsigned int mem32_serial_in(unsigned long addr, int offset)
> * early_pci_serial_init()
> *
> * This function is invoked when the early_printk param starts with "pciserial"
> - * The rest of the param should be ",B:D.F,baud" where B, D & F describe the
> - * location of a PCI device that must be a UART device.
> + * The rest of the param should be ",B:D.F,baud" or ",B:D.F,force,baud", where
> + * B, D & F describe the location of a PCI device that must be a UART device,
> + * "force" is optional and means insisting using a UART device with a wrong
> + * pci class code.
> */
> static __init void early_pci_serial_init(char *s)
> {
> @@ -224,6 +226,7 @@ static __init void early_pci_serial_init(char *s)
> u32 classcode, bar0;
> u16 cmdreg;
> char *e;
> + int force = 0;
>
>
> /*
> @@ -252,6 +255,15 @@ static __init void early_pci_serial_init(char *s)
> if (*s == ',')
> s++;
>
> + /* User may insist to use a UART device with wrong class code */
> + if (!strncmp(s, "force", 5)) {
> + force = 1;
> + s += 5;
> +
> + if (*s == ',')
> + s++;
No, you need to force the presence of "," otherwise cmdlines like this:
earlyprintk=pciserial,0:XX.X,forcedoodoo
work too.
> + }
> +
> /*
> * Second, find the device from the BDF
> */
> @@ -262,10 +274,12 @@ static __init void early_pci_serial_init(char *s)
> /*
> * Verify it is a UART type device
> */
> - if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> - (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> - (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
> - return;
> + if (!force) {
> + if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> + (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> + (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
> + return;
Move the force check in here ^
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-09-29 16:34 ` Borislav Petkov
@ 2018-09-30 13:16 ` Feng Tang
2018-10-01 12:39 ` Borislav Petkov
0 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2018-09-30 13:16 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
Hi Borislav,
On Sat, Sep 29, 2018 at 06:34:58PM +0200, Borislav Petkov wrote:
> On Fri, Sep 28, 2018 at 05:40:08PM +0800, Feng Tang wrote:
> > "pciserial" earlyprintk helps much on many modern x86 platforms,
> > but unfortunately there are still some platforms whose PCI UART
> > devices have wrong PCI class code, which will be blocked by current
> > class code check.
> >
> > Add a option "force" so that developer could still use a UART device
> > even it has wrong class code, with format "",B:D.F,force,baud". And
>
> This new parameter and its meaning needs to be documented where
> pciserial is documented.
Ok, will change the kernel-parameters.txt in next version.
>
> > the original format ",B:D.F,baud" is kept unchanged.
> >
> > Suggested-by: Borislav Petkov <bp@alien8.de>
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
> > arch/x86/kernel/early_printk.c | 26 ++++++++++++++++++++------
> > 1 file changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> > index 5e801c8..35d0f66 100644
> > --- a/arch/x86/kernel/early_printk.c
> > +++ b/arch/x86/kernel/early_printk.c
> > @@ -213,8 +213,10 @@ static unsigned int mem32_serial_in(unsigned long addr, int offset)
> > * early_pci_serial_init()
> > *
> > * This function is invoked when the early_printk param starts with "pciserial"
> > - * The rest of the param should be ",B:D.F,baud" where B, D & F describe the
> > - * location of a PCI device that must be a UART device.
> > + * The rest of the param should be ",B:D.F,baud" or ",B:D.F,force,baud", where
> > + * B, D & F describe the location of a PCI device that must be a UART device,
> > + * "force" is optional and means insisting using a UART device with a wrong
> > + * pci class code.
> > */
> > static __init void early_pci_serial_init(char *s)
> > {
> > @@ -224,6 +226,7 @@ static __init void early_pci_serial_init(char *s)
> > u32 classcode, bar0;
> > u16 cmdreg;
> > char *e;
> > + int force = 0;
> >
> >
> > /*
> > @@ -252,6 +255,15 @@ static __init void early_pci_serial_init(char *s)
> > if (*s == ',')
> > s++;
> >
> > + /* User may insist to use a UART device with wrong class code */
> > + if (!strncmp(s, "force", 5)) {
> > + force = 1;
> > + s += 5;
> > +
> > + if (*s == ',')
> > + s++;
>
> No, you need to force the presence of "," otherwise cmdlines like this:
>
> earlyprintk=pciserial,0:XX.X,forcedoodoo
>
> work too.
Got your point. As the following [,baudrate] is also optional, and this
"force" could be the last option for pciserial. The check may be
if (!strncmp(s, "force,", 6) || !strncmp(s, "force ", 6)) {
force = 1;
s += 6;
}
> > + }
> > +
> > /*
> > * Second, find the device from the BDF
> > */
> > @@ -262,10 +274,12 @@ static __init void early_pci_serial_init(char *s)
> > /*
> > * Verify it is a UART type device
> > */
> > - if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> > - (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> > - (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
> > - return;
> > + if (!force) {
> > + if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> > + (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> > + (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
> > + return;
>
> Move the force check in here ^
Do you mean this?
if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
(classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
(((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */ {
if (!force)
return;
}
Thanks,
Feng
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-09-30 13:16 ` Feng Tang
@ 2018-10-01 12:39 ` Borislav Petkov
2018-10-01 14:18 ` Feng Tang
0 siblings, 1 reply; 14+ messages in thread
From: Borislav Petkov @ 2018-10-01 12:39 UTC (permalink / raw)
To: Feng Tang
Cc: Thomas Gleixner, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
On Sun, Sep 30, 2018 at 09:16:19PM +0800, Feng Tang wrote:
> Got your point. As the following [,baudrate] is also optional, and this
> "force" could be the last option for pciserial. The check may be
>
> if (!strncmp(s, "force,", 6) || !strncmp(s, "force ", 6)) {
> force = 1;
> s += 6;
> }
You have !strncmp(s, "force,", 6) twice in there. I have no clue what
you're trying to say.
> Do you mean this?
>
> if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */ {
> if (!force)
> return;
> }
Yes.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-01 12:39 ` Borislav Petkov
@ 2018-10-01 14:18 ` Feng Tang
2018-10-01 20:30 ` Borislav Petkov
0 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2018-10-01 14:18 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
Hi Boris,
On Mon, Oct 01, 2018 at 02:39:16PM +0200, Borislav Petkov wrote:
> On Sun, Sep 30, 2018 at 09:16:19PM +0800, Feng Tang wrote:
> > Got your point. As the following [,baudrate] is also optional, and this
> > "force" could be the last option for pciserial. The check may be
> >
> > if (!strncmp(s, "force,", 6) || !strncmp(s, "force ", 6)) {
> > force = 1;
> > s += 6;
> > }
>
> You have !strncmp(s, "force,", 6) twice in there. I have no clue what
> you're trying to say.
I was trying to address your last comments:
"No, you need to force the presence of "," otherwise cmdlines like this:
earlyprintk=pciserial,0:XX.X,forcedoodoo
work too."
As I rechecked, the baud rate for pciserial is optional, so there may
be no ",baudrate" following the "force". So this 2 strncmp is to
cover conditions for with and without baudrate.
>
> > Do you mean this?
> >
> > if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> > (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> > (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */ {
> > if (!force)
> > return;
> > }
>
> Yes.
Ok, thanks
Thanks,
Feng
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-01 14:18 ` Feng Tang
@ 2018-10-01 20:30 ` Borislav Petkov
2018-10-02 9:16 ` Feng Tang
0 siblings, 1 reply; 14+ messages in thread
From: Borislav Petkov @ 2018-10-01 20:30 UTC (permalink / raw)
To: Feng Tang
Cc: Thomas Gleixner, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
On Mon, Oct 01, 2018 at 10:18:10PM +0800, Feng Tang wrote:
> As I rechecked, the baud rate for pciserial is optional, so there may
> be no ",baudrate" following the "force". So this 2 strncmp is to
> cover conditions for with and without baudrate.
And what guarantees you have a space after the "force"?
!strncmp(s, "force ", 6)
^
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-01 20:30 ` Borislav Petkov
@ 2018-10-02 9:16 ` Feng Tang
2018-10-02 9:17 ` Thomas Gleixner
0 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2018-10-02 9:16 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
Hi Boris,
On Mon, Oct 01, 2018 at 10:30:04PM +0200, Borislav Petkov wrote:
> On Mon, Oct 01, 2018 at 10:18:10PM +0800, Feng Tang wrote:
> > As I rechecked, the baud rate for pciserial is optional, so there may
> > be no ",baudrate" following the "force". So this 2 strncmp is to
> > cover conditions for with and without baudrate.
>
> And what guarantees you have a space after the "force"?
>
> !strncmp(s, "force ", 6)
> ^
You are right, it can't be guranteed. Can we still use strncmp(s, "force", 5)
and rely on developer to set it right? any suggestions? thanks,
- Feng
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-02 9:16 ` Feng Tang
@ 2018-10-02 9:17 ` Thomas Gleixner
2018-10-02 9:48 ` Feng Tang
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Gleixner @ 2018-10-02 9:17 UTC (permalink / raw)
To: Feng Tang
Cc: Borislav Petkov, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
On Tue, 2 Oct 2018, Feng Tang wrote:
> Hi Boris,
>
> On Mon, Oct 01, 2018 at 10:30:04PM +0200, Borislav Petkov wrote:
> > On Mon, Oct 01, 2018 at 10:18:10PM +0800, Feng Tang wrote:
> > > As I rechecked, the baud rate for pciserial is optional, so there may
> > > be no ",baudrate" following the "force". So this 2 strncmp is to
> > > cover conditions for with and without baudrate.
> >
> > And what guarantees you have a space after the "force"?
> >
> > !strncmp(s, "force ", 6)
> > ^
>
> You are right, it can't be guranteed. Can we still use strncmp(s, "force", 5)
> and rely on developer to set it right? any suggestions? thanks,
I don't know why you want strncmp() in the first place. "force" is null
terminated already.
Thanks,
tglx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-02 9:17 ` Thomas Gleixner
@ 2018-10-02 9:48 ` Feng Tang
2018-10-02 10:44 ` Thomas Gleixner
0 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2018-10-02 9:48 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Borislav Petkov, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
Hi Thomas,
On Tue, Oct 02, 2018 at 11:17:57AM +0200, Thomas Gleixner wrote:
> On Tue, 2 Oct 2018, Feng Tang wrote:
>
> > Hi Boris,
> >
> > On Mon, Oct 01, 2018 at 10:30:04PM +0200, Borislav Petkov wrote:
> > > On Mon, Oct 01, 2018 at 10:18:10PM +0800, Feng Tang wrote:
> > > > As I rechecked, the baud rate for pciserial is optional, so there may
> > > > be no ",baudrate" following the "force". So this 2 strncmp is to
> > > > cover conditions for with and without baudrate.
> > >
> > > And what guarantees you have a space after the "force"?
> > >
> > > !strncmp(s, "force ", 6)
> > > ^
> >
> > You are right, it can't be guranteed. Can we still use strncmp(s, "force", 5)
> > and rely on developer to set it right? any suggestions? thanks,
>
> I don't know why you want strncmp() in the first place. "force" is null
> terminated already.
Current code uses:
earlyprintk=pciserial,bus:device.function[,baudrate]
with the patch, it will be:
earlyprintk=pciserial,bus:device.function[,force][,baudrate]
So the force could be followed by ",baudrate".
Thanks,
Feng
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-02 9:48 ` Feng Tang
@ 2018-10-02 10:44 ` Thomas Gleixner
2018-10-02 12:10 ` Feng Tang
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Gleixner @ 2018-10-02 10:44 UTC (permalink / raw)
To: Feng Tang
Cc: Borislav Petkov, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
On Tue, 2 Oct 2018, Feng Tang wrote:
> On Tue, Oct 02, 2018 at 11:17:57AM +0200, Thomas Gleixner wrote:
> > On Tue, 2 Oct 2018, Feng Tang wrote:
> >
> > > Hi Boris,
> > >
> > > On Mon, Oct 01, 2018 at 10:30:04PM +0200, Borislav Petkov wrote:
> > > > On Mon, Oct 01, 2018 at 10:18:10PM +0800, Feng Tang wrote:
> > > > > As I rechecked, the baud rate for pciserial is optional, so there may
> > > > > be no ",baudrate" following the "force". So this 2 strncmp is to
> > > > > cover conditions for with and without baudrate.
> > > >
> > > > And what guarantees you have a space after the "force"?
> > > >
> > > > !strncmp(s, "force ", 6)
> > > > ^
> > >
> > > You are right, it can't be guranteed. Can we still use strncmp(s, "force", 5)
> > > and rely on developer to set it right? any suggestions? thanks,
> >
> > I don't know why you want strncmp() in the first place. "force" is null
> > terminated already.
>
> Current code uses:
> earlyprintk=pciserial,bus:device.function[,baudrate]
> with the patch, it will be:
> earlyprintk=pciserial,bus:device.function[,force][,baudrate]
>
> So the force could be followed by ",baudrate".
Sure, but that has nothing to do with strncmp(). The earlyprintk argument
string is tokenized by commata. It really does not matter where the 'force'
string is, but it matters that you check whether it is 'force' and not
'force$RANDOMCHARACTERS'. That's why strncmp() is the wrong thing to do,
because it only compares the first 5 characters and ignores anything
beyond, unless you make sure that the command line part is exactly 5
characters long.
Thanks,
tglx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-02 10:44 ` Thomas Gleixner
@ 2018-10-02 12:10 ` Feng Tang
2018-10-02 15:31 ` Feng Tang
0 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2018-10-02 12:10 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Borislav Petkov, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
Hi Thomas,
On Tue, Oct 02, 2018 at 12:44:36PM +0200, Thomas Gleixner wrote:
> On Tue, 2 Oct 2018, Feng Tang wrote:
> > On Tue, Oct 02, 2018 at 11:17:57AM +0200, Thomas Gleixner wrote:
> > > On Tue, 2 Oct 2018, Feng Tang wrote:
> > >
> > > > Hi Boris,
> > > >
> > > > On Mon, Oct 01, 2018 at 10:30:04PM +0200, Borislav Petkov wrote:
> > > > > On Mon, Oct 01, 2018 at 10:18:10PM +0800, Feng Tang wrote:
> > > > > > As I rechecked, the baud rate for pciserial is optional, so there may
> > > > > > be no ",baudrate" following the "force". So this 2 strncmp is to
> > > > > > cover conditions for with and without baudrate.
> > > > >
> > > > > And what guarantees you have a space after the "force"?
> > > > >
> > > > > !strncmp(s, "force ", 6)
> > > > > ^
> > > >
> > > > You are right, it can't be guranteed. Can we still use strncmp(s, "force", 5)
> > > > and rely on developer to set it right? any suggestions? thanks,
> > >
> > > I don't know why you want strncmp() in the first place. "force" is null
> > > terminated already.
> >
> > Current code uses:
> > earlyprintk=pciserial,bus:device.function[,baudrate]
> > with the patch, it will be:
> > earlyprintk=pciserial,bus:device.function[,force][,baudrate]
> >
> > So the force could be followed by ",baudrate".
>
> Sure, but that has nothing to do with strncmp(). The earlyprintk argument
> string is tokenized by commata. It really does not matter where the 'force'
> string is, but it matters that you check whether it is 'force' and not
> 'force$RANDOMCHARACTERS'. That's why strncmp() is the wrong thing to do,
> because it only compares the first 5 characters and ignores anything
> beyond, unless you make sure that the command line part is exactly 5
> characters long.
I don't know if I get it right due to my poor English, so for both cases
of "xxx,force,baudrate" and "xxx,force", it could be covered by
if ((!strcmp(s, "force") || !strncmp(s, "force," 6))
force = 1;
Thanks,
Feng
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-02 12:10 ` Feng Tang
@ 2018-10-02 15:31 ` Feng Tang
2018-10-02 15:41 ` Thomas Gleixner
0 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2018-10-02 15:31 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Borislav Petkov, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
On Tue, Oct 02, 2018 at 08:10:46PM +0800, Feng Tang wrote:
> Hi Thomas,
>
> On Tue, Oct 02, 2018 at 12:44:36PM +0200, Thomas Gleixner wrote:
> > On Tue, 2 Oct 2018, Feng Tang wrote:
> > > On Tue, Oct 02, 2018 at 11:17:57AM +0200, Thomas Gleixner wrote:
> > > > On Tue, 2 Oct 2018, Feng Tang wrote:
> > > >
> > > > > Hi Boris,
> > > > >
> > > > > On Mon, Oct 01, 2018 at 10:30:04PM +0200, Borislav Petkov wrote:
> > > > > > On Mon, Oct 01, 2018 at 10:18:10PM +0800, Feng Tang wrote:
> > > > > > > As I rechecked, the baud rate for pciserial is optional, so there may
> > > > > > > be no ",baudrate" following the "force". So this 2 strncmp is to
> > > > > > > cover conditions for with and without baudrate.
> > > > > >
> > > > > > And what guarantees you have a space after the "force"?
> > > > > >
> > > > > > !strncmp(s, "force ", 6)
> > > > > > ^
> > > > >
> > > > > You are right, it can't be guranteed. Can we still use strncmp(s, "force", 5)
> > > > > and rely on developer to set it right? any suggestions? thanks,
> > > >
> > > > I don't know why you want strncmp() in the first place. "force" is null
> > > > terminated already.
> > >
> > > Current code uses:
> > > earlyprintk=pciserial,bus:device.function[,baudrate]
> > > with the patch, it will be:
> > > earlyprintk=pciserial,bus:device.function[,force][,baudrate]
> > >
> > > So the force could be followed by ",baudrate".
> >
> > Sure, but that has nothing to do with strncmp(). The earlyprintk argument
> > string is tokenized by commata. It really does not matter where the 'force'
> > string is, but it matters that you check whether it is 'force' and not
> > 'force$RANDOMCHARACTERS'. That's why strncmp() is the wrong thing to do,
> > because it only compares the first 5 characters and ignores anything
> > beyond, unless you make sure that the command line part is exactly 5
> > characters long.
>
> I don't know if I get it right due to my poor English, so for both cases
> of "xxx,force,baudrate" and "xxx,force", it could be covered by
>
> if ((!strcmp(s, "force") || !strncmp(s, "force," 6))
> force = 1;
One idea is to change the new format to
earlyprintk=pciserial[,force],bus:device.function[,baudrate]
Then we can use what Boris has suggested:
!strncmp(s, "force,", 6)
Any thoughts?
Thanks,
Feng
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-02 15:31 ` Feng Tang
@ 2018-10-02 15:41 ` Thomas Gleixner
2018-10-02 15:49 ` Feng Tang
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Gleixner @ 2018-10-02 15:41 UTC (permalink / raw)
To: Feng Tang
Cc: Borislav Petkov, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
On Tue, 2 Oct 2018, Feng Tang wrote:
> One idea is to change the new format to
> earlyprintk=pciserial[,force],bus:device.function[,baudrate]
> Then we can use what Boris has suggested:
> !strncmp(s, "force,", 6)
Works for me.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device
2018-10-02 15:41 ` Thomas Gleixner
@ 2018-10-02 15:49 ` Feng Tang
0 siblings, 0 replies; 14+ messages in thread
From: Feng Tang @ 2018-10-02 15:49 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Borislav Petkov, Ingo Molnar, H Peter Anvin, Peter Zijlstra,
Stuart R . Anderson, alan, x86, linux-kernel
On Tue, Oct 02, 2018 at 05:41:39PM +0200, Thomas Gleixner wrote:
> On Tue, 2 Oct 2018, Feng Tang wrote:
> > One idea is to change the new format to
> > earlyprintk=pciserial[,force],bus:device.function[,baudrate]
> > Then we can use what Boris has suggested:
> > !strncmp(s, "force,", 6)
>
> Works for me.
Thanks, will send a v3.
- Feng
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-10-02 15:44 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-28 9:40 [PATCH v2] x86/earlyprintk: Add a force option for pciserial device Feng Tang
2018-09-29 16:34 ` Borislav Petkov
2018-09-30 13:16 ` Feng Tang
2018-10-01 12:39 ` Borislav Petkov
2018-10-01 14:18 ` Feng Tang
2018-10-01 20:30 ` Borislav Petkov
2018-10-02 9:16 ` Feng Tang
2018-10-02 9:17 ` Thomas Gleixner
2018-10-02 9:48 ` Feng Tang
2018-10-02 10:44 ` Thomas Gleixner
2018-10-02 12:10 ` Feng Tang
2018-10-02 15:31 ` Feng Tang
2018-10-02 15:41 ` Thomas Gleixner
2018-10-02 15:49 ` Feng Tang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).