Decode the hubless UVsystab passed from BIOS to the kernel saving
pertinent info in a similar manner that hubbed UVsystabs are decoded.
Signed-off-by: Mike Travis <[email protected]>
Reviewed-by: Steve Wahl <[email protected]>
Reviewed-by: Dimitri Sivanich <[email protected]>
To: Thomas Gleixner <[email protected]>
To: Ingo Molnar <[email protected]>
To: H. Peter Anvin <[email protected]>
To: Andrew Morton <[email protected]>
To: Borislav Petkov <[email protected]>
To: Christoph Hellwig <[email protected]>
Cc: Dimitri Sivanich <[email protected]>
Cc: Russ Anderson <[email protected]>
Cc: Hedi Berriche <[email protected]>
Cc: Steve Wahl <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
arch/x86/kernel/apic/x2apic_uv_x.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
--- linux.orig/arch/x86/kernel/apic/x2apic_uv_x.c
+++ linux/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -1303,7 +1303,8 @@ static int __init decode_uv_systab(void)
struct uv_systab *st;
int i;
- if (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE)
+ /* Select only UV4 (hubbed or hubless) and higher */
+ if (is_uv_hubbed(-2) < uv(4) && is_uv_hubless(-2) < uv(4))
return 0; /* No extended UVsystab required */
st = uv_systab;
@@ -1554,8 +1555,19 @@ static __init int uv_system_init_hubless
/* Init kernel/BIOS interface */
rc = uv_bios_init();
+ if (rc < 0) {
+ pr_err("UV: BIOS init error:%d\n", rc);
+ return rc;
+ }
+
+ /* Process UVsystab */
+ rc = decode_uv_systab();
+ if (rc < 0) {
+ pr_err("UV: UVsystab decode error:%d\n", rc);
+ return rc;
+ }
- /* Create user access node if UVsystab available */
+ /* Create user access node */
if (rc >= 0)
uv_setup_proc_files(1);
--
On Thu, Sep 05, 2019 at 08:02:58AM -0500, Mike Travis wrote:
> Decode the hubless UVsystab passed from BIOS to the kernel saving
> pertinent info in a similar manner that hubbed UVsystabs are decoded.
>
> Signed-off-by: Mike Travis <[email protected]>
> Reviewed-by: Steve Wahl <[email protected]>
> Reviewed-by: Dimitri Sivanich <[email protected]>
> To: Thomas Gleixner <[email protected]>
> To: Ingo Molnar <[email protected]>
> To: H. Peter Anvin <[email protected]>
> To: Andrew Morton <[email protected]>
> To: Borislav Petkov <[email protected]>
> To: Christoph Hellwig <[email protected]>
> Cc: Dimitri Sivanich <[email protected]>
> Cc: Russ Anderson <[email protected]>
> Cc: Hedi Berriche <[email protected]>
> Cc: Steve Wahl <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> arch/x86/kernel/apic/x2apic_uv_x.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
If you are trying to get one of my automated "WTF: patch XXXX was
seriously submitted to be applied to the stable tree?" emails, you are
on track for it...
Please go read the documentation link I sent you last time and figure
out how you can justify any of this patch series for a stable kernel
tree.
Also, nit:
> --- linux.orig/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ linux/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -1303,7 +1303,8 @@ static int __init decode_uv_systab(void)
> struct uv_systab *st;
> int i;
>
> - if (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE)
> + /* Select only UV4 (hubbed or hubless) and higher */
> + if (is_uv_hubbed(-2) < uv(4) && is_uv_hubless(-2) < uv(4))
> return 0; /* No extended UVsystab required */
>
> st = uv_systab;
> @@ -1554,8 +1555,19 @@ static __init int uv_system_init_hubless
>
> /* Init kernel/BIOS interface */
> rc = uv_bios_init();
> + if (rc < 0) {
> + pr_err("UV: BIOS init error:%d\n", rc);
Why isn't that function printing an error?
> + return rc;
> + }
> +
> + /* Process UVsystab */
> + rc = decode_uv_systab();
> + if (rc < 0) {
> + pr_err("UV: UVsystab decode error:%d\n", rc);
Same here, have the function itself print the error, makes this type of
stuff much cleaner.
greg k-h
On 9/5/2019 7:16 AM, Greg KH wrote:
> On Thu, Sep 05, 2019 at 08:02:58AM -0500, Mike Travis wrote:
>> Decode the hubless UVsystab passed from BIOS to the kernel saving
>> pertinent info in a similar manner that hubbed UVsystabs are decoded.
>>
>> Signed-off-by: Mike Travis <[email protected]>
>> Reviewed-by: Steve Wahl <[email protected]>
>> Reviewed-by: Dimitri Sivanich <[email protected]>
>> To: Thomas Gleixner <[email protected]>
>> To: Ingo Molnar <[email protected]>
>> To: H. Peter Anvin <[email protected]>
>> To: Andrew Morton <[email protected]>
>> To: Borislav Petkov <[email protected]>
>> To: Christoph Hellwig <[email protected]>
>> Cc: Dimitri Sivanich <[email protected]>
>> Cc: Russ Anderson <[email protected]>
>> Cc: Hedi Berriche <[email protected]>
>> Cc: Steve Wahl <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> arch/x86/kernel/apic/x2apic_uv_x.c | 16 ++++++++++++++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> If you are trying to get one of my automated "WTF: patch XXXX was
> seriously submitted to be applied to the stable tree?" emails, you are
> on track for it...
>
> Please go read the documentation link I sent you last time and figure
> out how you can justify any of this patch series for a stable kernel
> tree.
Is it because it has fixes for new hardware? If so, then I'll quit
submitting them to stable (we've had requests from distros for all
updates be in the stable tree for acceptance). Otherwise I thought it
does comply with:
" - To have the patch automatically included in the stable tree,
add the tag
Cc: [email protected]
in the sign-off area. Once the patch is merged it will be applied
to the stable tree without anything else needing to be done by the
author or subsystem maintainer."
Or is there some other reason that I'm not understanding?
>
> Also, nit:
>
>> --- linux.orig/arch/x86/kernel/apic/x2apic_uv_x.c
>> +++ linux/arch/x86/kernel/apic/x2apic_uv_x.c
>> @@ -1303,7 +1303,8 @@ static int __init decode_uv_systab(void)
>> struct uv_systab *st;
>> int i;
>>
>> - if (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE)
>> + /* Select only UV4 (hubbed or hubless) and higher */
>> + if (is_uv_hubbed(-2) < uv(4) && is_uv_hubless(-2) < uv(4))
>> return 0; /* No extended UVsystab required */
>>
>> st = uv_systab;
>> @@ -1554,8 +1555,19 @@ static __init int uv_system_init_hubless
>>
>> /* Init kernel/BIOS interface */
>> rc = uv_bios_init();
>> + if (rc < 0) {
>> + pr_err("UV: BIOS init error:%d\n", rc);
>
> Why isn't that function printing an error?
>
>
>> + return rc;
>> + }
>> +
>> + /* Process UVsystab */
>> + rc = decode_uv_systab();
>> + if (rc < 0) {
>> + pr_err("UV: UVsystab decode error:%d\n", rc);
>
> Same here, have the function itself print the error, makes this type of
> stuff much cleaner.
You're right this would be much cleaner. Mostly this was done because
of the rarity of an error here, and the specifics (BIOS failures)
usually cannot be dealt with by users. The system log is captured as
part of the error and packaged with other fault details that are
analyzed internally.
But I will make the changes you are suggesting. And thanks.
>
> greg k-h
>
On 9/5/2019 7:47 AM, Mike Travis wrote:
> Also, nit:
>
>> --- linux.orig/arch/x86/kernel/apic/x2apic_uv_x.c
>> +++ linux/arch/x86/kernel/apic/x2apic_uv_x.c
>> @@ -1303,7 +1303,8 @@ static int __init decode_uv_systab(void)
>> ????? struct uv_systab *st;
>> ????? int i;
>> -??? if (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE)
>> +??? /* Select only UV4 (hubbed or hubless) and higher */
>> +??? if (is_uv_hubbed(-2) < uv(4) && is_uv_hubless(-2) < uv(4))
>> ????????? return 0;??? /* No extended UVsystab required */
>> ????? st = uv_systab;
>> @@ -1554,8 +1555,19 @@ static __init int uv_system_init_hubless
>> ????? /* Init kernel/BIOS interface */
>> ????? rc = uv_bios_init();
>> +??? if (rc < 0) {
>> +??????? pr_err("UV: BIOS init error:%d\n", rc);
>
> Why isn't that function printing an error?
>
>
>> +??????? return rc;
>> +??? }
>> +
>> +??? /* Process UVsystab */
>> +??? rc = decode_uv_systab();
>> +??? if (rc < 0) {
>> +??????? pr_err("UV: UVsystab decode error:%d\n", rc);
>
> Same here, have the function itself print the error, makes this type of
> stuff much cleaner.
Turns out both functions already print an error message for each
instance of an error. The only redundancy is the caller also printing
an error with just the numeric error code. Shall I remove that?
On Thu, Sep 05, 2019 at 07:47:34AM -0700, Mike Travis wrote:
>
>
> On 9/5/2019 7:16 AM, Greg KH wrote:
> > On Thu, Sep 05, 2019 at 08:02:58AM -0500, Mike Travis wrote:
> > > Decode the hubless UVsystab passed from BIOS to the kernel saving
> > > pertinent info in a similar manner that hubbed UVsystabs are decoded.
> > >
> > > Signed-off-by: Mike Travis <[email protected]>
> > > Reviewed-by: Steve Wahl <[email protected]>
> > > Reviewed-by: Dimitri Sivanich <[email protected]>
> > > To: Thomas Gleixner <[email protected]>
> > > To: Ingo Molnar <[email protected]>
> > > To: H. Peter Anvin <[email protected]>
> > > To: Andrew Morton <[email protected]>
> > > To: Borislav Petkov <[email protected]>
> > > To: Christoph Hellwig <[email protected]>
> > > Cc: Dimitri Sivanich <[email protected]>
> > > Cc: Russ Anderson <[email protected]>
> > > Cc: Hedi Berriche <[email protected]>
> > > Cc: Steve Wahl <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > ---
> > > arch/x86/kernel/apic/x2apic_uv_x.c | 16 ++++++++++++++--
> > > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > If you are trying to get one of my automated "WTF: patch XXXX was
> > seriously submitted to be applied to the stable tree?" emails, you are
> > on track for it...
> >
> > Please go read the documentation link I sent you last time and figure
> > out how you can justify any of this patch series for a stable kernel
> > tree.
>
> Is it because it has fixes for new hardware? If so, then I'll quit
> submitting them to stable (we've had requests from distros for all updates
> be in the stable tree for acceptance). Otherwise I thought it does comply
> with:
>
> " - To have the patch automatically included in the stable tree,
> add the tag
> Cc: [email protected]
> in the sign-off area. Once the patch is merged it will be applied
> to the stable tree without anything else needing to be done by the
> author or subsystem maintainer."
>
> Or is there some other reason that I'm not understanding?
Yes, that's how you get a patch applied, but how in the world does all
of the patches in this series actually meet the requirements of a patch
that should be applied to the stable kernel tree?
I see no regression fixes, no new device ids, no bug fixes. Only
support for new hardware, i.e. a new feature to the kernel for something
that never worked in the first place.
And yes, distros do request bugfixes to get added to stable trees,
that's great, but I fail to understand how any of these patches are "bug
fixes". Maybe you need to work on your changelog texts...
good luck!
greg k-h
On Thu, Sep 05, 2019 at 09:43:57AM -0700, Mike Travis wrote:
>
>
> On 9/5/2019 7:47 AM, Mike Travis wrote:
> > Also, nit:
> >
> > > --- linux.orig/arch/x86/kernel/apic/x2apic_uv_x.c
> > > +++ linux/arch/x86/kernel/apic/x2apic_uv_x.c
> > > @@ -1303,7 +1303,8 @@ static int __init decode_uv_systab(void)
> > > ????? struct uv_systab *st;
> > > ????? int i;
> > > -??? if (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE)
> > > +??? /* Select only UV4 (hubbed or hubless) and higher */
> > > +??? if (is_uv_hubbed(-2) < uv(4) && is_uv_hubless(-2) < uv(4))
> > > ????????? return 0;??? /* No extended UVsystab required */
> > > ????? st = uv_systab;
> > > @@ -1554,8 +1555,19 @@ static __init int uv_system_init_hubless
> > > ????? /* Init kernel/BIOS interface */
> > > ????? rc = uv_bios_init();
> > > +??? if (rc < 0) {
> > > +??????? pr_err("UV: BIOS init error:%d\n", rc);
> >
> > Why isn't that function printing an error?
> >
> >
> > > +??????? return rc;
> > > +??? }
> > > +
> > > +??? /* Process UVsystab */
> > > +??? rc = decode_uv_systab();
> > > +??? if (rc < 0) {
> > > +??????? pr_err("UV: UVsystab decode error:%d\n", rc);
> >
> > Same here, have the function itself print the error, makes this type of
> > stuff much cleaner.
>
> Turns out both functions already print an error message for each instance of
> an error. The only redundancy is the caller also printing an error with
> just the numeric error code. Shall I remove that?
Of course you should, why would you want to see multiple error messages
for the same single error?
greg k-h
On Thu, Sep 05, 2019 at 04:16:34PM +0200, Greg KH wrote:
>On Thu, Sep 05, 2019 at 08:02:58AM -0500, Mike Travis wrote:
>> --- linux.orig/arch/x86/kernel/apic/x2apic_uv_x.c
>> +++ linux/arch/x86/kernel/apic/x2apic_uv_x.c
>> @@ -1303,7 +1303,8 @@ static int __init decode_uv_systab(void)
>> struct uv_systab *st;
>> int i;
>>
>> - if (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE)
>> + /* Select only UV4 (hubbed or hubless) and higher */
>> + if (is_uv_hubbed(-2) < uv(4) && is_uv_hubless(-2) < uv(4))
For someone not too familiar with the code, this is completely
unreadable. There must be a nicer way to do this.
--
Thanks,
Sasha
On 9/5/2019 2:40 PM, Sasha Levin wrote:
> On Thu, Sep 05, 2019 at 04:16:34PM +0200, Greg KH wrote:
>> On Thu, Sep 05, 2019 at 08:02:58AM -0500, Mike Travis wrote:
>>> --- linux.orig/arch/x86/kernel/apic/x2apic_uv_x.c
>>> +++ linux/arch/x86/kernel/apic/x2apic_uv_x.c
>>> @@ -1303,7 +1303,8 @@ static int __init decode_uv_systab(void)
>>> ???? struct uv_systab *st;
>>> ???? int i;
>>>
>>> -??? if (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE)
>>> +??? /* Select only UV4 (hubbed or hubless) and higher */
>>> +??? if (is_uv_hubbed(-2) < uv(4) && is_uv_hubless(-2) < uv(4))
>
> For someone not too familiar with the code, this is completely
> unreadable. There must be a nicer way to do this.
>
> --
> Thanks,
> Sasha
Hi Sasha,
I can put in further explanation but first the uv() function returns 1
left shifted by the UV #:
static inline int uv(int uvtype)
{
/* uv(0) is "any" */
if (uvtype >= 0 && uvtype <= 30)
return 1 << uvtype;
return 1;
}
The "is_uv_hubbed(x)" and "is_uv_hubless(x)" AND's the incoming arg with
the actual uv type:
int is_uv_hubbed(int uvtype)
{
return (uv_hubbed_system & uvtype);
}
The uv_hub{bed,less}_system is set to 1 left shifted by the UV # plus in
bit 0 is a '1' to indicate "any" UV (as in "is_uv_hubbed(1)" is any UV
hubbed system). Hubbed indicates a hubbed system, and hubless indicates
a hubless system, it cannot be both but can be neither.
> /* UV4 Hubless, (0x11:UV4+Any) */
> if (strncmp(oem_id, "NSGI4", 5) == 0)
> uv_hubless_system = 0x11;
>
> /* UV3 Hubless, UV300/MC990X w/o hub (0x9:UV3+Any) */
> else
> uv_hubless_system = 0x9;
>
(There are only hubbed versions of UV1 and UV2.)
Lastly (-2) translates to 0xffff...fffe (note bit 0 is clear to avoid
the "any" bit. So it is looking for a a hubbed or hubless UV system
that is less than UV4 meaning only UV4,5,6...qualify, hence this comment:
>>> + /* Select only UV4 (hubbed or hubless) and higher */
if (UV is less than UV4 either hubbed or hubless)
return; /* does not have an extended UVsystab */
Have you a suggestion on what would make it more clear? Perhaps instead
of -2 I should use a hex mask?
Thanks,
Mike