[PATCH 0/3] apci, nfit: DSM improvements

[PATCH 0/3] apci, nfit: DSM improvements

[Linda Knippers]

The first two patches in this series are motivated by feedback
from distribution developers, test engineers, and management tool
developers.  We need the ability to test and support Linux and 
management tools on systems that support more than one DSM family
for NVDIMM-N.  

We also need the ability to test and support new functions without
requiring users to update their kernel.  These changes will also
facilate development as we move toward DSM standardization.
The ability to restrict DSM functions to the currently documented
set is still in place.

The third patch cleans up a nit.

Linda Knippers (3):
  Allow all supported HPE DSM functions to be called
  Allow specifying a default DSM family
  Remove unnecessary newline

 drivers/acpi/nfit/core.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

As it is today, we can't enable or test new functions in firmware without
changing the kernel.

With this patch we allow function 0 for the HPE DSM families,
as is already allowed with the MS family. We now only restrict
the functions to the currently documented set if the module parameter
"disable_vendor_specific" is set.  Since the module parameter
description says "Limit commands to the publicly specified set",
this approach seems correct.

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 7361d00..b80d4d9 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1407,11 +1407,13 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 		if (disable_vendor_specific)
 			dsm_mask &= ~(1 << ND_CMD_VENDOR);
 	} else if (nfit_mem->family == NVDIMM_FAMILY_HPE1) {
-		dsm_mask = 0x1c3c76;
+		dsm_mask = 0xffffffff;
+		if (disable_vendor_specific)
+			dsm_mask &= ~(0x1c3c77);
 	} else if (nfit_mem->family == NVDIMM_FAMILY_HPE2) {
-		dsm_mask = 0x1fe;
+		dsm_mask = 0xffffffff;
 		if (disable_vendor_specific)
-			dsm_mask &= ~(1 << 8);
+			dsm_mask &= ~(0x0ff);
 	} else if (nfit_mem->family == NVDIMM_FAMILY_MSFT) {
 		dsm_mask = 0xffffffff;
 	} else {
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> As it is today, we can't enable or test new functions in firmware without
> changing the kernel.
>
> With this patch we allow function 0 for the HPE DSM families,
> as is already allowed with the MS family. We now only restrict
> the functions to the currently documented set if the module parameter
> "disable_vendor_specific" is set.  Since the module parameter
> description says "Limit commands to the publicly specified set",
> this approach seems correct.
>

My concern is that this makes the kernel less strict by default. Given
this is only a test capability lets add a module option to override
the default dsm_mask. That would also seem to combine well with the
new 'default_dsm_family' option.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 02/13/2017 12:28 PM, Dan Williams wrote:
> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> As it is today, we can't enable or test new functions in firmware without
>> changing the kernel.
>>
>> With this patch we allow function 0 for the HPE DSM families,
>> as is already allowed with the MS family. We now only restrict
>> the functions to the currently documented set if the module parameter
>> "disable_vendor_specific" is set.  Since the module parameter
>> description says "Limit commands to the publicly specified set",
>> this approach seems correct.
>>
> 
> My concern is that this makes the kernel less strict by default. Given
> this is only a test capability lets add a module option to override
> the default dsm_mask. 

This part isn't strictly a test capability.  It's also to allow older
kernels to be supported with newer NVDIMM hardware, firmware, and management tools.
We shouldn't need a customer to update their production kernel just to support
an NVDIMM management tool.

As for less secure by default, the default is to allow undocumented
functions of the "vendor specific" type.  How is the really different?

-- ljk

> That would also seem to combine well with the
> new 'default_dsm_family' option.


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Mon, Feb 13, 2017 at 10:17 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 02/13/2017 12:28 PM, Dan Williams wrote:
>> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>> As it is today, we can't enable or test new functions in firmware without
>>> changing the kernel.
>>>
>>> With this patch we allow function 0 for the HPE DSM families,
>>> as is already allowed with the MS family. We now only restrict
>>> the functions to the currently documented set if the module parameter
>>> "disable_vendor_specific" is set.  Since the module parameter
>>> description says "Limit commands to the publicly specified set",
>>> this approach seems correct.
>>>
>>
>> My concern is that this makes the kernel less strict by default. Given
>> this is only a test capability lets add a module option to override
>> the default dsm_mask.
>
> This part isn't strictly a test capability.  It's also to allow older
> kernels to be supported with newer NVDIMM hardware, firmware, and management tools.
> We shouldn't need a customer to update their production kernel just to support
> an NVDIMM management tool.
>
> As for less secure by default, the default is to allow undocumented
> functions of the "vendor specific" type.  How is the really different?
>

It's about pushing back on undocumented BIOS interfaces as much as
possible. The kernel has the documented set enabled by default,
including the documented vendor-specifc function code. There is the
option to disable the vendor-specific tunnel to restrict the kernel to
only allowing commands with publicly documented effects. With a new
"default_dsm_mask" parameter the default kernel policy can be
overridden by the system owner.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 02/13/2017 01:30 PM, Dan Williams wrote:
> On Mon, Feb 13, 2017 at 10:17 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> On 02/13/2017 12:28 PM, Dan Williams wrote:
>>> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>> As it is today, we can't enable or test new functions in firmware without
>>>> changing the kernel.
>>>>
>>>> With this patch we allow function 0 for the HPE DSM families,
>>>> as is already allowed with the MS family. We now only restrict
>>>> the functions to the currently documented set if the module parameter
>>>> "disable_vendor_specific" is set.  Since the module parameter
>>>> description says "Limit commands to the publicly specified set",
>>>> this approach seems correct.
>>>>
>>>
>>> My concern is that this makes the kernel less strict by default. Given
>>> this is only a test capability lets add a module option to override
>>> the default dsm_mask.
>>
>> This part isn't strictly a test capability.  It's also to allow older
>> kernels to be supported with newer NVDIMM hardware, firmware, and management tools.
>> We shouldn't need a customer to update their production kernel just to support
>> an NVDIMM management tool.
>>
>> As for less secure by default, the default is to allow undocumented
>> functions of the "vendor specific" type.  How is the really different?
>>
> 
> It's about pushing back on undocumented BIOS interfaces as much as
> possible. 

I'm not looking to propagate a bunch of undocumented interfaces.  I'm
looking to avoid having distros release new kernels and customers update
their systems because there's a new documented interface, especially when the only
thing that needs to change is the mask.  As you've experienced, the standardization
process takes a long time and I'm anticipating that between now and
eventual standardization, there will be at least one new DSM that's needed.
I'm sure we'll document it too, but that won't help a customer running an
older kernel.

> The kernel has the documented set enabled by default,
> including the documented vendor-specifc function code. 

Ok, so if I update my document to say that undefined values are vendor-specific,
we're good?  Or shall I define the unused values that way explicitly?

> There is the option to disable the vendor-specific tunnel to restrict the kernel to
> only allowing commands with publicly documented effects. 

My patch respects that option.

> With a new "default_dsm_mask" parameter the default kernel policy can be
> overridden by the system owner.

It can, although it means customers would have to add kernel parameters, which
they try to avoid.  I assume that's why the module parameter for a stricter
policy isn't the default since customers using Intel management tools would need to
add a parameter to allow the use of the vendor-specific function.

I'm not trying to be difficult here, but I really don't see the difference
between a documented function with undocumented behavior and an
undocumented function with undocumented behavior.

-- ljk


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Wed, Feb 15, 2017 at 3:19 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 02/13/2017 01:30 PM, Dan Williams wrote:
>> On Mon, Feb 13, 2017 at 10:17 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>> On 02/13/2017 12:28 PM, Dan Williams wrote:
>>>> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>> As it is today, we can't enable or test new functions in firmware without
>>>>> changing the kernel.
>>>>>
>>>>> With this patch we allow function 0 for the HPE DSM families,
>>>>> as is already allowed with the MS family. We now only restrict
>>>>> the functions to the currently documented set if the module parameter
>>>>> "disable_vendor_specific" is set.  Since the module parameter
>>>>> description says "Limit commands to the publicly specified set",
>>>>> this approach seems correct.
>>>>>
>>>>
>>>> My concern is that this makes the kernel less strict by default. Given
>>>> this is only a test capability lets add a module option to override
>>>> the default dsm_mask.
>>>
>>> This part isn't strictly a test capability.  It's also to allow older
>>> kernels to be supported with newer NVDIMM hardware, firmware, and management tools.
>>> We shouldn't need a customer to update their production kernel just to support
>>> an NVDIMM management tool.
>>>
>>> As for less secure by default, the default is to allow undocumented
>>> functions of the "vendor specific" type.  How is the really different?
>>>
>>
>> It's about pushing back on undocumented BIOS interfaces as much as
>> possible.
>
> I'm not looking to propagate a bunch of undocumented interfaces.  I'm
> looking to avoid having distros release new kernels and customers update
> their systems because there's a new documented interface, especially when the only
> thing that needs to change is the mask.  As you've experienced, the standardization
> process takes a long time and I'm anticipating that between now and
> eventual standardization, there will be at least one new DSM that's needed.
> I'm sure we'll document it too, but that won't help a customer running an
> older kernel.
>
>> The kernel has the documented set enabled by default,
>> including the documented vendor-specifc function code.
>
> Ok, so if I update my document to say that undefined values are vendor-specific,
> we're good?  Or shall I define the unused values that way explicitly?
>
>> There is the option to disable the vendor-specific tunnel to restrict the kernel to
>> only allowing commands with publicly documented effects.
>
> My patch respects that option.
>
>> With a new "default_dsm_mask" parameter the default kernel policy can be
>> overridden by the system owner.
>
> It can, although it means customers would have to add kernel parameters, which
> they try to avoid.  I assume that's why the module parameter for a stricter
> policy isn't the default since customers using Intel management tools would need to
> add a parameter to allow the use of the vendor-specific function.
>
> I'm not trying to be difficult here, but I really don't see the difference
> between a documented function with undocumented behavior and an
> undocumented function with undocumented behavior.

The difference is that it is slightly more painful to require a kernel
module override. Disabling these commands by default has some effect
of pushing back on the creation of new interfaces. Customers are
already prepared to need new kernels for new enabling. It's a similar
coordination we do for new device-ids for drivers, but we still allow
for overriding the default via the "new_id" interface in sysfs.

Look at the reaction we got from the community the last time this
subject was brought up [1]. That reaction stems from the historical
divergence of storage management interfaces requiring Linux to have a
vendor-specific toolkit per device. Requiring a kernel change to make
a command supported by default is a good thing because it affords
ongoing opportunities to find commonalities between vendors and
support common tooling.

I hope the need for the vendor-specific tunnels goes away over time
and Linux can generically support the most common management tasks
with generic infrastructure.

[1]: https://lists.01.org/pipermail/linux-nvdimm/2016-April/005493.html
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 02/15/2017 07:29 PM, Dan Williams wrote:
> On Wed, Feb 15, 2017 at 3:19 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> On 02/13/2017 01:30 PM, Dan Williams wrote:
>>> On Mon, Feb 13, 2017 at 10:17 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>> On 02/13/2017 12:28 PM, Dan Williams wrote:
>>>>> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>>> As it is today, we can't enable or test new functions in firmware without
>>>>>> changing the kernel.
>>>>>>
>>>>>> With this patch we allow function 0 for the HPE DSM families,
>>>>>> as is already allowed with the MS family. We now only restrict
>>>>>> the functions to the currently documented set if the module parameter
>>>>>> "disable_vendor_specific" is set.  Since the module parameter
>>>>>> description says "Limit commands to the publicly specified set",
>>>>>> this approach seems correct.
>>>>>>
>>>>>
>>>>> My concern is that this makes the kernel less strict by default. Given
>>>>> this is only a test capability lets add a module option to override
>>>>> the default dsm_mask.
>>>>
>>>> This part isn't strictly a test capability.  It's also to allow older
>>>> kernels to be supported with newer NVDIMM hardware, firmware, and management tools.
>>>> We shouldn't need a customer to update their production kernel just to support
>>>> an NVDIMM management tool.
>>>>
>>>> As for less secure by default, the default is to allow undocumented
>>>> functions of the "vendor specific" type.  How is the really different?
>>>>
>>>
>>> It's about pushing back on undocumented BIOS interfaces as much as
>>> possible.
>>
>> I'm not looking to propagate a bunch of undocumented interfaces.  I'm
>> looking to avoid having distros release new kernels and customers update
>> their systems because there's a new documented interface, especially when the only
>> thing that needs to change is the mask.  As you've experienced, the standardization
>> process takes a long time and I'm anticipating that between now and
>> eventual standardization, there will be at least one new DSM that's needed.
>> I'm sure we'll document it too, but that won't help a customer running an
>> older kernel.
>>
>>> The kernel has the documented set enabled by default,
>>> including the documented vendor-specifc function code.
>>
>> Ok, so if I update my document to say that undefined values are vendor-specific,
>> we're good?  Or shall I define the unused values that way explicitly?
>>
>>> There is the option to disable the vendor-specific tunnel to restrict the kernel to
>>> only allowing commands with publicly documented effects.
>>
>> My patch respects that option.
>>
>>> With a new "default_dsm_mask" parameter the default kernel policy can be
>>> overridden by the system owner.
>>
>> It can, although it means customers would have to add kernel parameters, which
>> they try to avoid.  I assume that's why the module parameter for a stricter
>> policy isn't the default since customers using Intel management tools would need to
>> add a parameter to allow the use of the vendor-specific function.
>>
>> I'm not trying to be difficult here, but I really don't see the difference
>> between a documented function with undocumented behavior and an
>> undocumented function with undocumented behavior.
> 
> The difference is that it is slightly more painful to require a kernel
> module override. Disabling these commands by default has some effect
> of pushing back on the creation of new interfaces. 

Intel's vendor-specific function, which can expose new interfaces,
requires no override.

> Customers are
> already prepared to need new kernels for new enabling. 

This may not be new enabling.  It could be hardware they already have
but there is a FW update or a management feature or some error recovery
that gets turned on.

> It's a similar coordination we do for new device-ids for drivers, but we still allow
> for overriding the default via the "new_id" interface in sysfs.
> 
> Look at the reaction we got from the community the last time this
> subject was brought up [1]. 

I actually think we handled that objection in the next few messages.

> That reaction stems from the historical
> divergence of storage management interfaces requiring Linux to have a
> vendor-specific toolkit per device. Requiring a kernel change to make
> a command supported by default is a good thing because it affords
> ongoing opportunities to find commonalities between vendors and
> support common tooling.

A good thing for who?  Not for the distro or the customer.  I don't
see how this is different than Intel adding new features through your generic
vendor-specific function, which would not require a kernel spin.

> I hope the need for the vendor-specific tunnels goes away over time
> and Linux can generically support the most common management tasks
> with generic infrastructure.

We totally agree.  In case you're wondering, we don't have a new
DSM queued up just waiting to be exposed by this patch.  However, cases
have come up where we have considered the need for a new DSM function
outside of pure test environments.  So far we have come up with other
approaches but at some point, the right approach will be to add a new DSM
function and I'd really like to not have to spin the kernel and all that
goes along with that when it happens. It's not good for customers.

-- ljk
> 
> [1]: https://lists.01.org/pipermail/linux-nvdimm/2016-April/005493.html
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Wed, Feb 15, 2017 at 4:45 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 02/15/2017 07:29 PM, Dan Williams wrote:
>> On Wed, Feb 15, 2017 at 3:19 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>> On 02/13/2017 01:30 PM, Dan Williams wrote:
>>>> On Mon, Feb 13, 2017 at 10:17 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>> On 02/13/2017 12:28 PM, Dan Williams wrote:
>>>>>> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>>>> As it is today, we can't enable or test new functions in firmware without
>>>>>>> changing the kernel.
>>>>>>>
>>>>>>> With this patch we allow function 0 for the HPE DSM families,
>>>>>>> as is already allowed with the MS family. We now only restrict
>>>>>>> the functions to the currently documented set if the module parameter
>>>>>>> "disable_vendor_specific" is set.  Since the module parameter
>>>>>>> description says "Limit commands to the publicly specified set",
>>>>>>> this approach seems correct.
>>>>>>>
>>>>>>
>>>>>> My concern is that this makes the kernel less strict by default. Given
>>>>>> this is only a test capability lets add a module option to override
>>>>>> the default dsm_mask.
>>>>>
>>>>> This part isn't strictly a test capability.  It's also to allow older
>>>>> kernels to be supported with newer NVDIMM hardware, firmware, and management tools.
>>>>> We shouldn't need a customer to update their production kernel just to support
>>>>> an NVDIMM management tool.
>>>>>
>>>>> As for less secure by default, the default is to allow undocumented
>>>>> functions of the "vendor specific" type.  How is the really different?
>>>>>
>>>>
>>>> It's about pushing back on undocumented BIOS interfaces as much as
>>>> possible.
>>>
>>> I'm not looking to propagate a bunch of undocumented interfaces.  I'm
>>> looking to avoid having distros release new kernels and customers update
>>> their systems because there's a new documented interface, especially when the only
>>> thing that needs to change is the mask.  As you've experienced, the standardization
>>> process takes a long time and I'm anticipating that between now and
>>> eventual standardization, there will be at least one new DSM that's needed.
>>> I'm sure we'll document it too, but that won't help a customer running an
>>> older kernel.
>>>
>>>> The kernel has the documented set enabled by default,
>>>> including the documented vendor-specifc function code.
>>>
>>> Ok, so if I update my document to say that undefined values are vendor-specific,
>>> we're good?  Or shall I define the unused values that way explicitly?
>>>
>>>> There is the option to disable the vendor-specific tunnel to restrict the kernel to
>>>> only allowing commands with publicly documented effects.
>>>
>>> My patch respects that option.
>>>
>>>> With a new "default_dsm_mask" parameter the default kernel policy can be
>>>> overridden by the system owner.
>>>
>>> It can, although it means customers would have to add kernel parameters, which
>>> they try to avoid.  I assume that's why the module parameter for a stricter
>>> policy isn't the default since customers using Intel management tools would need to
>>> add a parameter to allow the use of the vendor-specific function.
>>>
>>> I'm not trying to be difficult here, but I really don't see the difference
>>> between a documented function with undocumented behavior and an
>>> undocumented function with undocumented behavior.
>>
>> The difference is that it is slightly more painful to require a kernel
>> module override. Disabling these commands by default has some effect
>> of pushing back on the creation of new interfaces.
>
> Intel's vendor-specific function, which can expose new interfaces,
> requires no override.
>

Right, and NVDIMM_FAMILY_HPE2 has one too, and we have the kernel
option to disable all vendor specific passthroughs if an environment
only wants to run commands with publicly documented effects.

There's also nothing on the kernel side stopping any vendor from
implementing the "HPE2" or "INTEL" vendor specific interfaces in their
BIOS, the interfaces are not exclusive.

>> Customers are
>> already prepared to need new kernels for new enabling.
>
> This may not be new enabling.  It could be hardware they already have
> but there is a FW update or a management feature or some error recovery
> that gets turned on.
>

I consider new platform functionality hardware or firmware as new enabling.

>> It's a similar coordination we do for new device-ids for drivers, but we still allow
>> for overriding the default via the "new_id" interface in sysfs.
>>
>> Look at the reaction we got from the community the last time this
>> subject was brought up [1].
>
> I actually think we handled that objection in the next few messages.
>

I think we placated it with the strict checking, but the fact that the
kernel continues to have 4 nvdimm-family-types is still a point of
contention.

>> That reaction stems from the historical
>> divergence of storage management interfaces requiring Linux to have a
>> vendor-specific toolkit per device. Requiring a kernel change to make
>> a command supported by default is a good thing because it affords
>> ongoing opportunities to find commonalities between vendors and
>> support common tooling.
>
> A good thing for who?  Not for the distro or the customer.  I don't
> see how this is different than Intel adding new features through your generic
> vendor-specific function, which would not require a kernel spin.

A distro would rather carry one utility that handles multiple vendors.
The kernel would rather carry support code for one generic interface
than multiple interfaces.  A kernel spin is not required with the
proposed module parameter.

>> I hope the need for the vendor-specific tunnels goes away over time
>> and Linux can generically support the most common management tasks
>> with generic infrastructure.
>
> We totally agree.  In case you're wondering, we don't have a new
> DSM queued up just waiting to be exposed by this patch.  However, cases
> have come up where we have considered the need for a new DSM function
> outside of pure test environments.  So far we have come up with other
> approaches but at some point, the right approach will be to add a new DSM
> function and I'd really like to not have to spin the kernel and all that
> goes along with that when it happens. It's not good for customers.
>

I don't understand why a module option is unacceptable when that is
the proposed solution to the kernel picking the wrong "default"
family.

In fact, thinking about it further, I'd be more open to a patch that
allows overriding a DIMM's family via sysfs. That way we can
potentially have cross usage of these different interfaces and it's
less awkward for tooling to use than a module option.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 2/15/2017 9:03 PM, Dan Williams wrote:
> On Wed, Feb 15, 2017 at 4:45 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> On 02/15/2017 07:29 PM, Dan Williams wrote:
>>> On Wed, Feb 15, 2017 at 3:19 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>> On 02/13/2017 01:30 PM, Dan Williams wrote:
>>>>> On Mon, Feb 13, 2017 at 10:17 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>>> On 02/13/2017 12:28 PM, Dan Williams wrote:
>>>>>>> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>>>>> As it is today, we can't enable or test new functions in firmware without
>>>>>>>> changing the kernel.
>>>>>>>>
>>>>>>>> With this patch we allow function 0 for the HPE DSM families,
>>>>>>>> as is already allowed with the MS family. We now only restrict
>>>>>>>> the functions to the currently documented set if the module parameter
>>>>>>>> "disable_vendor_specific" is set.  Since the module parameter
>>>>>>>> description says "Limit commands to the publicly specified set",
>>>>>>>> this approach seems correct.
>>>>>>>>
>>>>>>>
>>>>>>> My concern is that this makes the kernel less strict by default. Given
>>>>>>> this is only a test capability lets add a module option to override
>>>>>>> the default dsm_mask.
>>>>>>
>>>>>> This part isn't strictly a test capability.  It's also to allow older
>>>>>> kernels to be supported with newer NVDIMM hardware, firmware, and management tools.
>>>>>> We shouldn't need a customer to update their production kernel just to support
>>>>>> an NVDIMM management tool.
>>>>>>
>>>>>> As for less secure by default, the default is to allow undocumented
>>>>>> functions of the "vendor specific" type.  How is the really different?
>>>>>>
>>>>>
>>>>> It's about pushing back on undocumented BIOS interfaces as much as
>>>>> possible.
>>>>
>>>> I'm not looking to propagate a bunch of undocumented interfaces.  I'm
>>>> looking to avoid having distros release new kernels and customers update
>>>> their systems because there's a new documented interface, especially when the only
>>>> thing that needs to change is the mask.  As you've experienced, the standardization
>>>> process takes a long time and I'm anticipating that between now and
>>>> eventual standardization, there will be at least one new DSM that's needed.
>>>> I'm sure we'll document it too, but that won't help a customer running an
>>>> older kernel.
>>>>
>>>>> The kernel has the documented set enabled by default,
>>>>> including the documented vendor-specifc function code.
>>>>
>>>> Ok, so if I update my document to say that undefined values are vendor-specific,
>>>> we're good?  Or shall I define the unused values that way explicitly?
>>>>
>>>>> There is the option to disable the vendor-specific tunnel to restrict the kernel to
>>>>> only allowing commands with publicly documented effects.
>>>>
>>>> My patch respects that option.
>>>>
>>>>> With a new "default_dsm_mask" parameter the default kernel policy can be
>>>>> overridden by the system owner.
>>>>
>>>> It can, although it means customers would have to add kernel parameters, which
>>>> they try to avoid.  I assume that's why the module parameter for a stricter
>>>> policy isn't the default since customers using Intel management tools would need to
>>>> add a parameter to allow the use of the vendor-specific function.
>>>>
>>>> I'm not trying to be difficult here, but I really don't see the difference
>>>> between a documented function with undocumented behavior and an
>>>> undocumented function with undocumented behavior.
>>>
>>> The difference is that it is slightly more painful to require a kernel
>>> module override. Disabling these commands by default has some effect
>>> of pushing back on the creation of new interfaces.
>>
>> Intel's vendor-specific function, which can expose new interfaces,
>> requires no override.
>>
>
> Right, and NVDIMM_FAMILY_HPE2 has one too, and we have the kernel
> option to disable all vendor specific passthroughs if an environment
> only wants to run commands with publicly documented effects.

Ok, I can add some to the HPE1 family too.  :-)

> There's also nothing on the kernel side stopping any vendor from
> implementing the "HPE2" or "INTEL" vendor specific interfaces in their
> BIOS, the interfaces are not exclusive.

I'm sure that many vendors will implement the INTEL interfaces when
there are devices using the 0x0201 RFIC.  However, those aren't sufficient
for NVDIMM-N devices.

As for HPE2, it may be short lived.  I'm not sure it's in the wild and
if it's not, perhaps we can remove it.  Still checking.

>>> Customers are
>>> already prepared to need new kernels for new enabling.
>>
>> This may not be new enabling.  It could be hardware they already have
>> but there is a FW update or a management feature or some error recovery
>> that gets turned on.
>>
>
> I consider new platform functionality hardware or firmware as new enabling.

A customer may not see it that way.

>>> It's a similar coordination we do for new device-ids for drivers, but we still allow
>>> for overriding the default via the "new_id" interface in sysfs.
>>>
>>> Look at the reaction we got from the community the last time this
>>> subject was brought up [1].
>>
>> I actually think we handled that objection in the next few messages.
>>
>
> I think we placated it with the strict checking, but the fact that the
> kernel continues to have 4 nvdimm-family-types is still a point of
> contention.

We will likely end up supporting a 5th one as we move to a standard.

>>> That reaction stems from the historical
>>> divergence of storage management interfaces requiring Linux to have a
>>> vendor-specific toolkit per device. Requiring a kernel change to make
>>> a command supported by default is a good thing because it affords
>>> ongoing opportunities to find commonalities between vendors and
>>> support common tooling.
>>
>> A good thing for who?  Not for the distro or the customer.  I don't
>> see how this is different than Intel adding new features through your generic
>> vendor-specific function, which would not require a kernel spin.
>
> A distro would rather carry one utility that handles multiple vendors.
> The kernel would rather carry support code for one generic interface
> than multiple interfaces.

We'd all like that - but we don't have that yet.

> A kernel spin is not required with the proposed module parameter.

True, but configuring boot options and a reboot are required.

>>> I hope the need for the vendor-specific tunnels goes away over time
>>> and Linux can generically support the most common management tasks
>>> with generic infrastructure.
>>
>> We totally agree.  In case you're wondering, we don't have a new
>> DSM queued up just waiting to be exposed by this patch.  However, cases
>> have come up where we have considered the need for a new DSM function
>> outside of pure test environments.  So far we have come up with other
>> approaches but at some point, the right approach will be to add a new DSM
>> function and I'd really like to not have to spin the kernel and all that
>> goes along with that when it happens. It's not good for customers.
>>
>
> I don't understand why a module option is unacceptable when that is
> the proposed solution to the kernel picking the wrong "default"
> family.

The difference is that picking the default family is primarily for
testing while the other case it's not.  Testers do all sorts of things
that customers won't like.

> In fact, thinking about it further, I'd be more open to a patch that
> allows overriding a DIMM's family via sysfs.

Sorry if this is a dumb question but will that work at boot time?  Today
there are DSMs that are called during initialization.  There could also
be DSMs called based on whether NFIT device flag bit 4 is set, although
we don't currently do that.

> That way we can
> potentially have cross usage of these different interfaces and it's
> less awkward for tooling to use than a module option.

For testing purposes, being able to switch DSM families without a reboot
would be really nice.  You could even expose the GUID and allow it, the
family, and the dsm mask to be overridden. Would be helpful when testing
that new standard DSM we all aspire to.

This approach would touch a lot more code and require a lot more testing
than my relatively simple patches because today these things are configured
at boot time rather than run time.  I wonder about ordering and consistency
checking.  For example, if someone changed the family would you automatically 
change the mask or is that two operations?  I assume you'd check
that the family is actually supported for the device?  With sysfs, different
devices could have different families, where with the module parameter option
there's one default that's applied to all devices for which that family is
supported.

At this point I'd probably prefer the simplicity of the module parameter
because I know I can do it and test it.  If the only way you'll take the
patch is to add a second parameter then I'll do that, but I still don't
see the point.

-- ljk
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Thu, Feb 16, 2017 at 10:51 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> Right, and NVDIMM_FAMILY_HPE2 has one too, and we have the kernel
>> option to disable all vendor specific passthroughs if an environment
>> only wants to run commands with publicly documented effects.
>
>
> Ok, I can add some to the HPE1 family too.  :-)

Sure, and then the pushback would be "no, just use HPE2 or INTEL". We
have two kernel-supported methods for sending an opaque
vendor-specific command. Why would the kernel need to support a third?

>> There's also nothing on the kernel side stopping any vendor from
>> implementing the "HPE2" or "INTEL" vendor specific interfaces in their
>> BIOS, the interfaces are not exclusive.
>
>
> I'm sure that many vendors will implement the INTEL interfaces when
> there are devices using the 0x0201 RFIC.  However, those aren't sufficient
> for NVDIMM-N devices.
>
> As for HPE2, it may be short lived.  I'm not sure it's in the wild and
> if it's not, perhaps we can remove it.  Still checking.

This is an example of why the kernel does not add kernel enabling for
future "maybe" cases. The fact that this patch wants to allow the
kernel to maybe support a command that doesn't exist yet is a clear
reason to say no.

>>>> Customers are
>>>> already prepared to need new kernels for new enabling.
>>>
>>>
>>> This may not be new enabling.  It could be hardware they already have
>>> but there is a FW update or a management feature or some error recovery
>>> that gets turned on.
>>>
>>
>> I consider new platform functionality hardware or firmware as new
>> enabling.
>
>
> A customer may not see it that way.
>
>>>> It's a similar coordination we do for new device-ids for drivers, but we
>>>> still allow
>>>> for overriding the default via the "new_id" interface in sysfs.
>>>>
>>>> Look at the reaction we got from the community the last time this
>>>> subject was brought up [1].
>>>
>>>
>>> I actually think we handled that objection in the next few messages.
>>>
>>
>> I think we placated it with the strict checking, but the fact that the
>> kernel continues to have 4 nvdimm-family-types is still a point of
>> contention.
>
>
> We will likely end up supporting a 5th one as we move to a standard.
>
>>>> That reaction stems from the historical
>>>> divergence of storage management interfaces requiring Linux to have a
>>>> vendor-specific toolkit per device. Requiring a kernel change to make
>>>> a command supported by default is a good thing because it affords
>>>> ongoing opportunities to find commonalities between vendors and
>>>> support common tooling.
>>>
>>>
>>> A good thing for who?  Not for the distro or the customer.  I don't
>>> see how this is different than Intel adding new features through your
>>> generic
>>> vendor-specific function, which would not require a kernel spin.
>>
>>
>> A distro would rather carry one utility that handles multiple vendors.
>> The kernel would rather carry support code for one generic interface
>> than multiple interfaces.
>
>
> We'd all like that - but we don't have that yet.
>
>> A kernel spin is not required with the proposed module parameter.
>
>
> True, but configuring boot options and a reboot are required.

Module options are specified at module load time so you don't
necessarily need a reboot. However, I'm now of the opinion that
allowing the family to be changed is a more complete solution to both
problems.

>>>> I hope the need for the vendor-specific tunnels goes away over time
>>>> and Linux can generically support the most common management tasks
>>>> with generic infrastructure.
>>>
>>>
>>> We totally agree.  In case you're wondering, we don't have a new
>>> DSM queued up just waiting to be exposed by this patch.  However, cases
>>> have come up where we have considered the need for a new DSM function
>>> outside of pure test environments.  So far we have come up with other
>>> approaches but at some point, the right approach will be to add a new DSM
>>> function and I'd really like to not have to spin the kernel and all that
>>> goes along with that when it happens. It's not good for customers.
>>>
>>
>> I don't understand why a module option is unacceptable when that is
>> the proposed solution to the kernel picking the wrong "default"
>> family.
>
>
> The difference is that picking the default family is primarily for
> testing while the other case it's not.  Testers do all sorts of things
> that customers won't like.
>
>> In fact, thinking about it further, I'd be more open to a patch that
>> allows overriding a DIMM's family via sysfs.
>
>
> Sorry if this is a dumb question but will that work at boot time?  Today
> there are DSMs that are called during initialization.

The only DIMM-level DSMs that are called during initialization are
label management, those aren't the DSMs we are talking about here.

> There could also
> be DSMs called based on whether NFIT device flag bit 4 is set, although
> we don't currently do that.

Sorry, I'm not sure to which NFIT bit this is referring? I'm not sure
it matters if the kernel is never going to initiate and consume the
result of the DSM like it does label data.

>> That way we can
>> potentially have cross usage of these different interfaces and it's
>> less awkward for tooling to use than a module option.
>
>
> For testing purposes, being able to switch DSM families without a reboot
> would be really nice.  You could even expose the GUID and allow it, the
> family, and the dsm mask to be overridden. Would be helpful when testing
> that new standard DSM we all aspire to.
>
> This approach would touch a lot more code and require a lot more testing
> than my relatively simple patches because today these things are configured
> at boot time rather than run time.  I wonder about ordering and consistency
> checking.  For example, if someone changed the family would you
> automatically change the mask or is that two operations?  I assume you'd
> check
> that the family is actually supported for the device?  With sysfs, different
> devices could have different families, where with the module parameter
> option
> there's one default that's applied to all devices for which that family is
> supported.
>
> At this point I'd probably prefer the simplicity of the module parameter
> because I know I can do it and test it.  If the only way you'll take the
> patch is to add a second parameter then I'll do that, but I still don't
> see the point.

Let's do the work to allow the family to be switched so that tooling
can override the kernel default, because it does seem valid for a DIMM
to support multiple DSM-family types and that gives more than one path
to use a vendor specific passthrough.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 02/16/2017 02:35 PM, Dan Williams wrote:
> On Thu, Feb 16, 2017 at 10:51 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>> Right, and NVDIMM_FAMILY_HPE2 has one too, and we have the kernel
>>> option to disable all vendor specific passthroughs if an environment
>>> only wants to run commands with publicly documented effects.
>>
>>
>> Ok, I can add some to the HPE1 family too.  :-)
> 
> Sure, and then the pushback would be "no, just use HPE2 or INTEL". We
> have two kernel-supported methods for sending an opaque
> vendor-specific command. Why would the kernel need to support a third?

I can't use Intel's because I have an NVDIMM-N and your spec is not
for NVDIMM-N.

>>> There's also nothing on the kernel side stopping any vendor from
>>> implementing the "HPE2" or "INTEL" vendor specific interfaces in their
>>> BIOS, the interfaces are not exclusive.
>>
>>
>> I'm sure that many vendors will implement the INTEL interfaces when
>> there are devices using the 0x0201 RFIC.  However, those aren't sufficient
>> for NVDIMM-N devices.
>>
>> As for HPE2, it may be short lived.  I'm not sure it's in the wild and
>> if it's not, perhaps we can remove it.  Still checking.
> 
> This is an example of why the kernel does not add kernel enabling for
> future "maybe" cases. The fact that this patch wants to allow the
> kernel to maybe support a command that doesn't exist yet is a clear
> reason to say no.

The kernel often has code for devices that aren't available yet,
like NVDIMMs with 0x201. Sometimes things change or eventually get deprecated.
The Intel DSM spec has changed over time as well. But as I said, I'm still checking.

> 
>>>>> Customers are
>>>>> already prepared to need new kernels for new enabling.
>>>>
>>>>
>>>> This may not be new enabling.  It could be hardware they already have
>>>> but there is a FW update or a management feature or some error recovery
>>>> that gets turned on.
>>>>
>>>
>>> I consider new platform functionality hardware or firmware as new
>>> enabling.
>>
>>
>> A customer may not see it that way.
>>
>>>>> It's a similar coordination we do for new device-ids for drivers, but we
>>>>> still allow
>>>>> for overriding the default via the "new_id" interface in sysfs.
>>>>>
>>>>> Look at the reaction we got from the community the last time this
>>>>> subject was brought up [1].
>>>>
>>>>
>>>> I actually think we handled that objection in the next few messages.
>>>>
>>>
>>> I think we placated it with the strict checking, but the fact that the
>>> kernel continues to have 4 nvdimm-family-types is still a point of
>>> contention.
>>
>>
>> We will likely end up supporting a 5th one as we move to a standard.
>>
>>>>> That reaction stems from the historical
>>>>> divergence of storage management interfaces requiring Linux to have a
>>>>> vendor-specific toolkit per device. Requiring a kernel change to make
>>>>> a command supported by default is a good thing because it affords
>>>>> ongoing opportunities to find commonalities between vendors and
>>>>> support common tooling.
>>>>
>>>>
>>>> A good thing for who?  Not for the distro or the customer.  I don't
>>>> see how this is different than Intel adding new features through your
>>>> generic
>>>> vendor-specific function, which would not require a kernel spin.
>>>
>>>
>>> A distro would rather carry one utility that handles multiple vendors.
>>> The kernel would rather carry support code for one generic interface
>>> than multiple interfaces.
>>
>>
>> We'd all like that - but we don't have that yet.
>>
>>> A kernel spin is not required with the proposed module parameter.
>>
>>
>> True, but configuring boot options and a reboot are required.
> 
> Module options are specified at module load time so you don't
> necessarily need a reboot. 

Reloading nfit has been a problem for me with dependencies between
other modules but perhaps I've just not gotten the sequence right.
Does it work for you?

> However, I'm now of the opinion that
> allowing the family to be changed is a more complete solution to both
> problems.
> 
>>>>> I hope the need for the vendor-specific tunnels goes away over time
>>>>> and Linux can generically support the most common management tasks
>>>>> with generic infrastructure.
>>>>
>>>>
>>>> We totally agree.  In case you're wondering, we don't have a new
>>>> DSM queued up just waiting to be exposed by this patch.  However, cases
>>>> have come up where we have considered the need for a new DSM function
>>>> outside of pure test environments.  So far we have come up with other
>>>> approaches but at some point, the right approach will be to add a new DSM
>>>> function and I'd really like to not have to spin the kernel and all that
>>>> goes along with that when it happens. It's not good for customers.
>>>>
>>>
>>> I don't understand why a module option is unacceptable when that is
>>> the proposed solution to the kernel picking the wrong "default"
>>> family.
>>
>>
>> The difference is that picking the default family is primarily for
>> testing while the other case it's not.  Testers do all sorts of things
>> that customers won't like.
>>
>>> In fact, thinking about it further, I'd be more open to a patch that
>>> allows overriding a DIMM's family via sysfs.
>>
>>
>> Sorry if this is a dumb question but will that work at boot time?  Today
>> there are DSMs that are called during initialization.
> 
> The only DIMM-level DSMs that are called during initialization are
> label management, those aren't the DSMs we are talking about here.
> 
>> There could also
>> be DSMs called based on whether NFIT device flag bit 4 is set, although
>> we don't currently do that.
> 
> Sorry, I'm not sure to which NFIT bit this is referring? I'm not sure
> it matters if the kernel is never going to initiate and consume the
> result of the DSM like it does label data.

There's a state flag that says there was a health status change prior
to the OSPM handoff.  We could eventually want to call that to know more
about whether the NVDIMM is viable for use as there is more information there
than in the basic state flags.  We don't do that today though, just
thinking about the future.

> 
>>> That way we can
>>> potentially have cross usage of these different interfaces and it's
>>> less awkward for tooling to use than a module option.
>>
>>
>> For testing purposes, being able to switch DSM families without a reboot
>> would be really nice.  You could even expose the GUID and allow it, the
>> family, and the dsm mask to be overridden. Would be helpful when testing
>> that new standard DSM we all aspire to.
>>
>> This approach would touch a lot more code and require a lot more testing
>> than my relatively simple patches because today these things are configured
>> at boot time rather than run time.  I wonder about ordering and consistency
>> checking.  For example, if someone changed the family would you
>> automatically change the mask or is that two operations?  I assume you'd
>> check
>> that the family is actually supported for the device?  With sysfs, different
>> devices could have different families, where with the module parameter
>> option
>> there's one default that's applied to all devices for which that family is
>> supported.
>>
>> At this point I'd probably prefer the simplicity of the module parameter
>> because I know I can do it and test it.  If the only way you'll take the
>> patch is to add a second parameter then I'll do that, but I still don't
>> see the point.
> 
> Let's do the work to allow the family to be switched so that tooling
> can override the kernel default, because it does seem valid for a DIMM
> to support multiple DSM-family types and that gives more than one path
> to use a vendor specific passthrough.

I am fairly certain that for production use we wouldn't expect management
tools to use more than one DSM family at a time. Tools using DSMs
from one family wouldn't switch to using a new family for one operation
and then switch back for another operation.

We haven't talked about the 4th DSM family and what their plans are but
I won't be shocked if they create new functions.  That family is very
hardware specific and hardware specs also evolve.

-- ljk

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Thu, Feb 16, 2017 at 12:13 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 02/16/2017 02:35 PM, Dan Williams wrote:
>> On Thu, Feb 16, 2017 at 10:51 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>> Right, and NVDIMM_FAMILY_HPE2 has one too, and we have the kernel
>>>> option to disable all vendor specific passthroughs if an environment
>>>> only wants to run commands with publicly documented effects.
>>>
>>>
>>> Ok, I can add some to the HPE1 family too.  :-)
>>
>> Sure, and then the pushback would be "no, just use HPE2 or INTEL". We
>> have two kernel-supported methods for sending an opaque
>> vendor-specific command. Why would the kernel need to support a third?
>
> I can't use Intel's because I have an NVDIMM-N and your spec is not
> for NVDIMM-N.

I don't understand this comment in the context of a vendor specific
tunnel. There's nothing NVDIMM-N specific about the DSM payload
format. I'm saying override the default family temporarily just to use
the vendor-specific tunnel, not implement the full "INTEL" definition.
This can be used as a stop-gap when you find you need to support a new
command on a legacy kernel.

>>>> There's also nothing on the kernel side stopping any vendor from
>>>> implementing the "HPE2" or "INTEL" vendor specific interfaces in their
>>>> BIOS, the interfaces are not exclusive.
>>>
>>>
>>> I'm sure that many vendors will implement the INTEL interfaces when
>>> there are devices using the 0x0201 RFIC.  However, those aren't sufficient
>>> for NVDIMM-N devices.
>>>
>>> As for HPE2, it may be short lived.  I'm not sure it's in the wild and
>>> if it's not, perhaps we can remove it.  Still checking.
>>
>> This is an example of why the kernel does not add kernel enabling for
>> future "maybe" cases. The fact that this patch wants to allow the
>> kernel to maybe support a command that doesn't exist yet is a clear
>> reason to say no.
>
> The kernel often has code for devices that aren't available yet,
> like NVDIMMs with 0x201. Sometimes things change or eventually get deprecated.
> The Intel DSM spec has changed over time as well. But as I said, I'm still checking.

I'm not sure that's a good example because block-apertures are
described by ACPI, and the kernel has no specific enabling around the
format-interface-code. Whereas this patch to expand the dsm_mask is
planning ahead for DSMs that may not ever be defined.

[..]
>> Module options are specified at module load time so you don't
>> necessarily need a reboot.
>
> Reloading nfit has been a problem for me with dependencies between
> other modules but perhaps I've just not gotten the sequence right.
> Does it work for you?

Yes:

    ndctl disable-region all
    modprobe -r nfit

It's best to unmount any filesystems mounted on nvdimm namespaces
before disabling regions. There's an ongoing effort to make if safe
[1], but not all filesystems are prepared for their backing device to
disappear.

[1]: https://lists.01.org/pipermail/linux-nvdimm/2016-January/003797.html

>> However, I'm now of the opinion that
>> allowing the family to be changed is a more complete solution to both
>> problems.
>>
>>>>>> I hope the need for the vendor-specific tunnels goes away over time
>>>>>> and Linux can generically support the most common management tasks
>>>>>> with generic infrastructure.
>>>>>
>>>>>
>>>>> We totally agree.  In case you're wondering, we don't have a new
>>>>> DSM queued up just waiting to be exposed by this patch.  However, cases
>>>>> have come up where we have considered the need for a new DSM function
>>>>> outside of pure test environments.  So far we have come up with other
>>>>> approaches but at some point, the right approach will be to add a new DSM
>>>>> function and I'd really like to not have to spin the kernel and all that
>>>>> goes along with that when it happens. It's not good for customers.
>>>>>
>>>>
>>>> I don't understand why a module option is unacceptable when that is
>>>> the proposed solution to the kernel picking the wrong "default"
>>>> family.
>>>
>>>
>>> The difference is that picking the default family is primarily for
>>> testing while the other case it's not.  Testers do all sorts of things
>>> that customers won't like.
>>>
>>>> In fact, thinking about it further, I'd be more open to a patch that
>>>> allows overriding a DIMM's family via sysfs.
>>>
>>>
>>> Sorry if this is a dumb question but will that work at boot time?  Today
>>> there are DSMs that are called during initialization.
>>
>> The only DIMM-level DSMs that are called during initialization are
>> label management, those aren't the DSMs we are talking about here.
>>
>>> There could also
>>> be DSMs called based on whether NFIT device flag bit 4 is set, although
>>> we don't currently do that.
>>
>> Sorry, I'm not sure to which NFIT bit this is referring? I'm not sure
>> it matters if the kernel is never going to initiate and consume the
>> result of the DSM like it does label data.
>
> There's a state flag that says there was a health status change prior
> to the OSPM handoff.  We could eventually want to call that to know more
> about whether the NVDIMM is viable for use as there is more information there
> than in the basic state flags.  We don't do that today though, just
> thinking about the future.

The kernel passes that status and any health-event notification to
userspace. The kernel doesn't consume it directly because the health
status payload is not standardized across vendors, and it keeps the
kernel focused on the mechanism while the policy of what to do with a
health event is a userspace concern. This is the same scheme the
kernel has for handling health status alerts from disks.

>>>> That way we can
>>>> potentially have cross usage of these different interfaces and it's
>>>> less awkward for tooling to use than a module option.
>>>
>>>
>>> For testing purposes, being able to switch DSM families without a reboot
>>> would be really nice.  You could even expose the GUID and allow it, the
>>> family, and the dsm mask to be overridden. Would be helpful when testing
>>> that new standard DSM we all aspire to.
>>>
>>> This approach would touch a lot more code and require a lot more testing
>>> than my relatively simple patches because today these things are configured
>>> at boot time rather than run time.  I wonder about ordering and consistency
>>> checking.  For example, if someone changed the family would you
>>> automatically change the mask or is that two operations?  I assume you'd
>>> check
>>> that the family is actually supported for the device?  With sysfs, different
>>> devices could have different families, where with the module parameter
>>> option
>>> there's one default that's applied to all devices for which that family is
>>> supported.
>>>
>>> At this point I'd probably prefer the simplicity of the module parameter
>>> because I know I can do it and test it.  If the only way you'll take the
>>> patch is to add a second parameter then I'll do that, but I still don't
>>> see the point.
>>
>> Let's do the work to allow the family to be switched so that tooling
>> can override the kernel default, because it does seem valid for a DIMM
>> to support multiple DSM-family types and that gives more than one path
>> to use a vendor specific passthrough.
>
> I am fairly certain that for production use we wouldn't expect management
> tools to use more than one DSM family at a time. Tools using DSMs
> from one family wouldn't switch to using a new family for one operation
> and then switch back for another operation.

If the kernel makes it safe I don't see why this is a blocker,
especially if the goal is to reduce interface proliferation over time.

> We haven't talked about the 4th DSM family and what their plans are but
> I won't be shocked if they create new functions.  That family is very
> hardware specific and hardware specs also evolve.

Yes, and we can add support for new functions when and if they define
them, not in advance.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Thu, Feb 16, 2017 at 12:48 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Feb 16, 2017 at 12:13 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
[..]
>> I am fairly certain that for production use we wouldn't expect management
>> tools to use more than one DSM family at a time. Tools using DSMs
>> from one family wouldn't switch to using a new family for one operation
>> and then switch back for another operation.
>
> If the kernel makes it safe I don't see why this is a blocker,
> especially if the goal is to reduce interface proliferation over time.

In fact, we can make it even simpler than dynamically changing family,
just have ND_IOCTL_VENDOR always route to uuid
""4309ac30-0d11-11e4-9191-0800200c9a66" and function number 9. Then
that becomes the de facto method for Linux to issue a vendor-specific
command for any DIMM.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 2/16/2017 4:07 PM, Dan Williams wrote:
> On Thu, Feb 16, 2017 at 12:48 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Thu, Feb 16, 2017 at 12:13 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> [..]
>>> I am fairly certain that for production use we wouldn't expect management
>>> tools to use more than one DSM family at a time. Tools using DSMs
>>> from one family wouldn't switch to using a new family for one operation
>>> and then switch back for another operation.
>>
>> If the kernel makes it safe I don't see why this is a blocker,
>> especially if the goal is to reduce interface proliferation over time.
>
> In fact, we can make it even simpler than dynamically changing family,
> just have ND_IOCTL_VENDOR always route to uuid
> ""4309ac30-0d11-11e4-9191-0800200c9a66" and function number 9. Then
> that becomes the de facto method for Linux to issue a vendor-specific
> command for any DIMM.

Several problems.
That family is documented for RFIC 0x0201.
No NVDIMM-N platform supports that DSM family.
When platforms do support 0x0201, some will also support NVDIMM-N.  Since 
opcodes are not documented, how do would they not step on each other?

I don't think that works at all.

-- ljk


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Thu, Feb 16, 2017 at 2:40 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>
>
> On 2/16/2017 4:07 PM, Dan Williams wrote:
>>
>> On Thu, Feb 16, 2017 at 12:48 PM, Dan Williams <dan.j.williams@intel.com>
>> wrote:
>>>
>>> On Thu, Feb 16, 2017 at 12:13 PM, Linda Knippers <linda.knippers@hpe.com>
>>> wrote:
>>
>> [..]
>>>>
>>>> I am fairly certain that for production use we wouldn't expect
>>>> management
>>>> tools to use more than one DSM family at a time. Tools using DSMs
>>>> from one family wouldn't switch to using a new family for one operation
>>>> and then switch back for another operation.
>>>
>>>
>>> If the kernel makes it safe I don't see why this is a blocker,
>>> especially if the goal is to reduce interface proliferation over time.
>>
>>
>> In fact, we can make it even simpler than dynamically changing family,
>> just have ND_IOCTL_VENDOR always route to uuid
>> ""4309ac30-0d11-11e4-9191-0800200c9a66" and function number 9. Then
>> that becomes the de facto method for Linux to issue a vendor-specific
>> command for any DIMM.
>
>
> Several problems.
> That family is documented for RFIC 0x0201.
> No NVDIMM-N platform supports that DSM family.
> When platforms do support 0x0201, some will also support NVDIMM-N.  Since
> opcodes are not documented, how do would they not step on each other?
>
> I don't think that works at all.
>

I'm confused there is no correlation between FIC and DSM family, at
least not one Linux enforces.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 2/16/2017 5:43 PM, Dan Williams wrote:
> On Thu, Feb 16, 2017 at 2:40 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>
>>
>> On 2/16/2017 4:07 PM, Dan Williams wrote:
>>>
>>> On Thu, Feb 16, 2017 at 12:48 PM, Dan Williams <dan.j.williams@intel.com>
>>> wrote:
>>>>
>>>> On Thu, Feb 16, 2017 at 12:13 PM, Linda Knippers <linda.knippers@hpe.com>
>>>> wrote:
>>>
>>> [..]
>>>>>
>>>>> I am fairly certain that for production use we wouldn't expect
>>>>> management
>>>>> tools to use more than one DSM family at a time. Tools using DSMs
>>>>> from one family wouldn't switch to using a new family for one operation
>>>>> and then switch back for another operation.
>>>>
>>>>
>>>> If the kernel makes it safe I don't see why this is a blocker,
>>>> especially if the goal is to reduce interface proliferation over time.
>>>
>>>
>>> In fact, we can make it even simpler than dynamically changing family,
>>> just have ND_IOCTL_VENDOR always route to uuid
>>> ""4309ac30-0d11-11e4-9191-0800200c9a66" and function number 9. Then
>>> that becomes the de facto method for Linux to issue a vendor-specific
>>> command for any DIMM.
>>
>>
>> Several problems.
>> That family is documented for RFIC 0x0201.
>> No NVDIMM-N platform supports that DSM family.
>> When platforms do support 0x0201, some will also support NVDIMM-N.  Since
>> opcodes are not documented, how do would they not step on each other?
>>
>> I don't think that works at all.
>>
>
> I'm confused there is no correlation between FIC and DSM family,

Page 7 of version 1.3 of your DSM spec says:

Platforms that have the _DSM interface implemented, as outlined in this section, 
can support a NVDIMM region with Region Format Interface Code (RFIC) of 0x0201.

Other DSM documents reference RFIC 0x0101.

> at least not one Linux enforces.

That is true, but FW developers aren't being as loose.

-- ljk
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Dan Williams]

On Thu, Feb 16, 2017 at 2:47 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>> Several problems.
>>> That family is documented for RFIC 0x0201.
>>> No NVDIMM-N platform supports that DSM family.
>>> When platforms do support 0x0201, some will also support NVDIMM-N.  Since
>>> opcodes are not documented, how do would they not step on each other?
>>>
>>> I don't think that works at all.
>>>
>>
>> I'm confused there is no correlation between FIC and DSM family,
>
>
> Page 7 of version 1.3 of your DSM spec says:
>
> Platforms that have the _DSM interface implemented, as outlined in this
> section, can support a NVDIMM region with Region Format Interface Code
> (RFIC) of 0x0201.
>
> Other DSM documents reference RFIC 0x0101.
>
>> at least not one Linux enforces.
>
>
> That is true, but FW developers aren't being as loose.

Yes, they are. We have 3 0x101 DSM families.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 2/16/2017 5:49 PM, Dan Williams wrote:
> On Thu, Feb 16, 2017 at 2:47 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>> Several problems.
>>>> That family is documented for RFIC 0x0201.
>>>> No NVDIMM-N platform supports that DSM family.
>>>> When platforms do support 0x0201, some will also support NVDIMM-N.  Since
>>>> opcodes are not documented, how do would they not step on each other?
>>>>
>>>> I don't think that works at all.
>>>>
>>>
>>> I'm confused there is no correlation between FIC and DSM family,
>>
>>
>> Page 7 of version 1.3 of your DSM spec says:
>>
>> Platforms that have the _DSM interface implemented, as outlined in this
>> section, can support a NVDIMM region with Region Format Interface Code
>> (RFIC) of 0x0201.
>>
>> Other DSM documents reference RFIC 0x0101.
>>
>>> at least not one Linux enforces.
>>
>>
>> That is true, but FW developers aren't being as loose.
>
> Yes, they are. We have 3 0x101 DSM families.

And they're all implemented for 0x101 devices.

-- ljk
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Linda Knippers]

On 2/16/2017 3:48 PM, Dan Williams wrote:
> On Thu, Feb 16, 2017 at 12:13 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> On 02/16/2017 02:35 PM, Dan Williams wrote:
>>> On Thu, Feb 16, 2017 at 10:51 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>> Right, and NVDIMM_FAMILY_HPE2 has one too, and we have the kernel
>>>>> option to disable all vendor specific passthroughs if an environment
>>>>> only wants to run commands with publicly documented effects.
>>>>
>>>>
>>>> Ok, I can add some to the HPE1 family too.  :-)
>>>
>>> Sure, and then the pushback would be "no, just use HPE2 or INTEL". We
>>> have two kernel-supported methods for sending an opaque
>>> vendor-specific command. Why would the kernel need to support a third?
>>
>> I can't use Intel's because I have an NVDIMM-N and your spec is not
>> for NVDIMM-N.
>
> I don't understand this comment in the context of a vendor specific
> tunnel. There's nothing NVDIMM-N specific about the DSM payload
> format. I'm saying override the default family temporarily just to use
> the vendor-specific tunnel, not implement the full "INTEL" definition.
> This can be used as a stop-gap when you find you need to support a new
> command on a legacy kernel.

That presumes that the platform FW supports the INTEL DSM.  Today's
platforms that support NVDIMM-N do not.  It is quite likely that
even when platform do support NVDIMMs with RFIC 0x201, FW wouldn't
expose the DSM family if the hardware isn't actually installed.

>>>>> There's also nothing on the kernel side stopping any vendor from
>>>>> implementing the "HPE2" or "INTEL" vendor specific interfaces in their
>>>>> BIOS, the interfaces are not exclusive.
>>>>
>>>>
>>>> I'm sure that many vendors will implement the INTEL interfaces when
>>>> there are devices using the 0x0201 RFIC.  However, those aren't sufficient
>>>> for NVDIMM-N devices.
>>>>
>>>> As for HPE2, it may be short lived.  I'm not sure it's in the wild and
>>>> if it's not, perhaps we can remove it.  Still checking.
>>>
>>> This is an example of why the kernel does not add kernel enabling for
>>> future "maybe" cases. The fact that this patch wants to allow the
>>> kernel to maybe support a command that doesn't exist yet is a clear
>>> reason to say no.
>>
>> The kernel often has code for devices that aren't available yet,
>> like NVDIMMs with 0x201. Sometimes things change or eventually get deprecated.
>> The Intel DSM spec has changed over time as well. But as I said, I'm still checking.
>
> I'm not sure that's a good example because block-apertures are
> described by ACPI, and the kernel has no specific enabling around the
> format-interface-code. Whereas this patch to expand the dsm_mask is
> planning ahead for DSMs that may not ever be defined.

My point was that things change.  They sometimes change before hardware is
shipping.  That's why your spec is up to version 1.3 and still changing.
I'm just trying to acknowledge that change happens and plan for it.

BTW, we do have HPE2 in the wild so never mind about removing it.
>
> [..]
>>> Module options are specified at module load time so you don't
>>> necessarily need a reboot.
>>
>> Reloading nfit has been a problem for me with dependencies between
>> other modules but perhaps I've just not gotten the sequence right.
>> Does it work for you?
>
> Yes:
>
>     ndctl disable-region all
>     modprobe -r nfit

Thanks!

> It's best to unmount any filesystems mounted on nvdimm namespaces
> before disabling regions. There's an ongoing effort to make if safe
> [1], but not all filesystems are prepared for their backing device to
> disappear.
>
> [1]: https://lists.01.org/pipermail/linux-nvdimm/2016-January/003797.html
>
>>> However, I'm now of the opinion that
>>> allowing the family to be changed is a more complete solution to both
>>> problems.
>>>
>>>>>>> I hope the need for the vendor-specific tunnels goes away over time
>>>>>>> and Linux can generically support the most common management tasks
>>>>>>> with generic infrastructure.
>>>>>>
>>>>>>
>>>>>> We totally agree.  In case you're wondering, we don't have a new
>>>>>> DSM queued up just waiting to be exposed by this patch.  However, cases
>>>>>> have come up where we have considered the need for a new DSM function
>>>>>> outside of pure test environments.  So far we have come up with other
>>>>>> approaches but at some point, the right approach will be to add a new DSM
>>>>>> function and I'd really like to not have to spin the kernel and all that
>>>>>> goes along with that when it happens. It's not good for customers.
>>>>>>
>>>>>
>>>>> I don't understand why a module option is unacceptable when that is
>>>>> the proposed solution to the kernel picking the wrong "default"
>>>>> family.
>>>>
>>>>
>>>> The difference is that picking the default family is primarily for
>>>> testing while the other case it's not.  Testers do all sorts of things
>>>> that customers won't like.
>>>>
>>>>> In fact, thinking about it further, I'd be more open to a patch that
>>>>> allows overriding a DIMM's family via sysfs.
>>>>
>>>>
>>>> Sorry if this is a dumb question but will that work at boot time?  Today
>>>> there are DSMs that are called during initialization.
>>>
>>> The only DIMM-level DSMs that are called during initialization are
>>> label management, those aren't the DSMs we are talking about here.
>>>
>>>> There could also
>>>> be DSMs called based on whether NFIT device flag bit 4 is set, although
>>>> we don't currently do that.
>>>
>>> Sorry, I'm not sure to which NFIT bit this is referring? I'm not sure
>>> it matters if the kernel is never going to initiate and consume the
>>> result of the DSM like it does label data.
>>
>> There's a state flag that says there was a health status change prior
>> to the OSPM handoff.  We could eventually want to call that to know more
>> about whether the NVDIMM is viable for use as there is more information there
>> than in the basic state flags.  We don't do that today though, just
>> thinking about the future.
>
> The kernel passes that status and any health-event notification to
> userspace. The kernel doesn't consume it directly because the health
> status payload is not standardized across vendors, and it keeps the
> kernel focused on the mechanism while the policy of what to do with a
> health event is a userspace concern. This is the same scheme the
> kernel has for handling health status alerts from disks.
>
>>>>> That way we can
>>>>> potentially have cross usage of these different interfaces and it's
>>>>> less awkward for tooling to use than a module option.
>>>>
>>>>
>>>> For testing purposes, being able to switch DSM families without a reboot
>>>> would be really nice.  You could even expose the GUID and allow it, the
>>>> family, and the dsm mask to be overridden. Would be helpful when testing
>>>> that new standard DSM we all aspire to.
>>>>
>>>> This approach would touch a lot more code and require a lot more testing
>>>> than my relatively simple patches because today these things are configured
>>>> at boot time rather than run time.  I wonder about ordering and consistency
>>>> checking.  For example, if someone changed the family would you
>>>> automatically change the mask or is that two operations?  I assume you'd
>>>> check
>>>> that the family is actually supported for the device?  With sysfs, different
>>>> devices could have different families, where with the module parameter
>>>> option
>>>> there's one default that's applied to all devices for which that family is
>>>> supported.
>>>>
>>>> At this point I'd probably prefer the simplicity of the module parameter
>>>> because I know I can do it and test it.  If the only way you'll take the
>>>> patch is to add a second parameter then I'll do that, but I still don't
>>>> see the point.
>>>
>>> Let's do the work to allow the family to be switched so that tooling
>>> can override the kernel default, because it does seem valid for a DIMM
>>> to support multiple DSM-family types and that gives more than one path
>>> to use a vendor specific passthrough.
>>
>> I am fairly certain that for production use we wouldn't expect management
>> tools to use more than one DSM family at a time. Tools using DSMs
>> from one family wouldn't switch to using a new family for one operation
>> and then switch back for another operation.
>
> If the kernel makes it safe I don't see why this is a blocker,
> especially if the goal is to reduce interface proliferation over time.

See my earlier comments.  It would require more than the kernel
making it safe.  It gets even more complicated when a platform supports
an 0x201 NVDIMM and an NVDIMM-N at the same time.

We all share the goal of reducing interface proliferation.  We disagree
on the enforcement while we define more complete standards.

>> We haven't talked about the 4th DSM family and what their plans are but
>> I won't be shocked if they create new functions.  That family is very
>> hardware specific and hardware specs also evolve.
>
> Yes, and we can add support for new functions when and if they define
> them, not in advance.

The problem is timing.  New FW and tools can be in customers hands much
more quickly than an OS release.  If a vendor documents a function today
and you pull in a patch adding the bit tomorrow, it could still be a year
or more before that patch makes it into the distro that the customer is running.
That is the fundamental issue that I'm trying to address.

--ljk
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow all supported HPE DSM functions to be called

[Boaz Harrosh]

On 02/16/2017 09:35 PM, Dan Williams wrote:
> On Thu, Feb 16, 2017 at 10:51 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
<>
>> True, but configuring boot options and a reboot are required.
> 
> Module options are specified at module load time so you don't
> necessarily need a reboot. However, I'm now of the opinion that
> allowing the family to be changed is a more complete solution to both
> problems.
<>
> 
> Let's do the work to allow the family to be switched so that tooling
> can override the kernel default, because it does seem valid for a DIMM
> to support multiple DSM-family types and that gives more than one path
> to use a vendor specific passthrough.

BTW:
Any module-parameter is already accessible via sysfs. And can be made
writeable. If you need to run a small function on modification, that
is easy enough, just as any sysfs, give the parameter a function-pointer.

Even if you do an hot event, I'd rather it be as a module-parameter,
because of the already established path in sysfs.

Just my $0.017
Boaz

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/3] Allow specifying a default DSM family

[Linda Knippers]

Provide the ability to request a default DSM family. If it is not
supported, then fall back to the normal discovery order.

This is helpful for testing platforms that support multiple DSM families.
It will also allow administrators to request the DSM family that their
management tools support, which may not be the first one found using
the current discovery order.

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit/core.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index b80d4d9..e38773f 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -51,6 +51,12 @@
 MODULE_PARM_DESC(disable_vendor_specific,
 		"Limit commands to the publicly specified set\n");
 
+static int default_dsm_family = -1;
+module_param(default_dsm_family, int, S_IRUGO);
+MODULE_PARM_DESC(default_dsm_family,
+		"Try this DSM type first when identifying NVDIMM family");
+
+
 LIST_HEAD(acpi_descs);
 DEFINE_MUTEX(acpi_desc_lock);
 
@@ -1367,7 +1373,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	struct device *dev = acpi_desc->dev;
 	unsigned long dsm_mask;
 	const u8 *uuid;
-	int i;
+	int i = -1;
 
 	/* nfit test assumes 1:1 relationship between commands and dsms */
 	nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
@@ -1395,10 +1401,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	 * Until standardization materializes we need to consider 4
 	 * different command sets.  Note, that checking for function0 (bit0)
 	 * tells us if any commands are reachable through this uuid.
+	 * First check for a requested default.
 	 */
-	for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
-		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
-			break;
+	if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
+			default_dsm_family <= NVDIMM_FAMILY_MSFT) {
+		if (acpi_check_dsm(adev_dimm->handle,
+				to_nfit_uuid(default_dsm_family), 1, 1))
+			i = default_dsm_family;
+		else
+			dev_dbg(dev, "default_dsm_family %d not supported\n",
+				default_dsm_family);
+	}
+	if (i == -1) {
+		for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
+			if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
+					1, 1))
+				break;
+	}
 
 	/* limit the supported commands to those that are publicly documented */
 	nfit_mem->family = i;
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 3/3] Remove unnecessary newline

[Linda Knippers]

Newline in MODULE_PARM_DESC messes up the modprobe output so remove it.

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index e38773f..6c522c8 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -49,7 +49,7 @@
 static bool disable_vendor_specific;
 module_param(disable_vendor_specific, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_vendor_specific,
-		"Limit commands to the publicly specified set\n");
+		"Limit commands to the publicly specified set");
 
 static int default_dsm_family = -1;
 module_param(default_dsm_family, int, S_IRUGO);
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/3] Remove unnecessary newline

[Dan Williams]

On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> Newline in MODULE_PARM_DESC messes up the modprobe output so remove it.

Yes, looks good. When this "messes up" does that mean the output less
pretty, or do you know if it breaks scripts parsing the output? Just
want to be clear on the effects of this change.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/3] Remove unnecessary newline

[Linda Knippers]

On 02/13/2017 12:32 PM, Dan Williams wrote:
> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> Newline in MODULE_PARM_DESC messes up the modprobe output so remove it.
> 
> Yes, looks good. When this "messes up" does that mean the output less
> pretty, or do you know if it breaks scripts parsing the output? Just
> want to be clear on the effects of this change.

If you do a 'modinfo nfit', it puts "(bool)" on a separate line.
That's unlike all the other parameters.  If someone is parsing the output
of modinfo, they may not get the type.

Without my patch, you get this:

parm:           force_enable_dimms:Ignore _STA (ACPI DIMM device) status (bool)
parm:           scrub_timeout:Initial scrub timeout in seconds (uint)
parm:           scrub_overflow_abort:Number of times we overflow ARS results before abort (uint)
parm:           disable_vendor_specific:Limit commands to the publicly specified set
(bool)
parm:           default_dsm_family:Try this DSM type first when identifying NVDIMM family (int)

instead of this:

parm:           force_enable_dimms:Ignore _STA (ACPI DIMM device) status (bool)
parm:           scrub_timeout:Initial scrub timeout in seconds (uint)
parm:           scrub_overflow_abort:Number of times we overflow ARS results before abort (uint)
parm:           disable_vendor_specific:Limit commands to the publicly specified set (bool)
parm:           default_dsm_family:Try this DSM type first when identifying NVDIMM family (int)


-- ljk
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm