Re: [PATCH] acpi, nfit: fix acpi_check_dsm() vs zero functions implemented

[Jerry Hoemann <jerry.hoemann@hpe.com>]

On Tue, Jun 28, 2016 at 05:37:45PM +0800, Xiao Guangrong wrote:
> 
> 
> On 06/28/2016 02:58 AM, Dan Williams wrote:
> > On Mon, Jun 27, 2016 at 11:47 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> > > 
> > > 
> > > On 6/27/2016 2:06 PM, Dan Williams wrote:
> > > > On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> > > > > 
> > > > > On 6/24/2016 1:44 PM, Dan Williams wrote:
> > > > > > QEMU 2.6 implements nascent support for nvdimm DSMs. Depending on
> > > > > > configuration it may only implement the function0 dsm to indicate that
> > > > > > no other DSMs are available. Commit 31eca76ba2fc "nfit, libnvdimm:
> > > > > > limited/whitelisted dimm command marshaling mechanism" breaks QEMU, but
> > > > > > QEMU is spec compliant.  Per the spec the way to indicate that no
> > > > > > functions are supported is:
> > > > > > 
> > > > > >      If Function Index is zero, the return is a buffer containing one bit
> > > > > >      for each function index, starting with zero. Bit 0 indicates whether
> > > > > >      there is support for any functions other than function 0 for the
> > > > > >      specified UUID and Revision ID. If set to zero, no functions are
> > > > > >      supported (other than function zero) for the specified UUID and
> > > > > >      Revision ID.
> > > > > 
> > > > > The rest of that paragraph is:
> > > > > 
> > > > > If set to one, at least one additional function is supported. For all other bits
> > > > > in the buffer, a bit is set to zero to indicate if that function index is not
> > > > > supported for the specific UUID and Revision ID. (For example, bit 1 set to 0
> > > > > indicates that function index 1 is not supported for the specific UUID and
> > > > > Revision ID.)
> > > > > 
> > > > > > 
> > > > > > Update the nfit driver to determine the family (interface UUID) without
> > > > > > requiring the implementation to define any other functions, i.e.
> > > > > > short-circuit acpi_check_dsm() to succeed per the spec.  The nfit driver
> > > > > > appears to be the only user passing funcs==0 to acpi_check_dsm(), so
> > > > > > this behavior change of the common routine should be limited to the
> > > > > > probing done by the nfit driver.
> > > > > 
> > > > > I don't understand why we're special casing this to support QEMU only when
> > > > > there are no DSM functions supported.  If we want to implement the
> > > > > spec and support function zero, I think we should support it correctly.
> > > > > That means returning the correct value for all spec compliant callers,
> > > > > even when there are functions that are supported.
> > > > 
> > > > QEMU 2.6 already shipped, so whatever we do we should not regress
> > > > those binaries.  The QEMU behavior could be argued as not spec
> > > > compliant, but they've implemented enough of function0 to answer the
> > > > "which family" probe.
> > > 
> > > How would you argue that?
> > 
> > acpi_evaluate_dsm() returns data instead of an error.
> > 
> > > > Yes, if an implementation supports function0 it
> > > > should say so in the returned bitmask,
> > > 
> > > But in other places you explicitly prevent function 0 from
> > > being executed.
> > 
> > Right, for the whitelist-filtered result to userspace, but this patch
> > is about the initial kernel probe.
> > 
> > > > but by the time we've
> > > > determined that function0 is "not supported" we've already
> > > > successfully executed a function0 request.
> > > 
> > > If they advertise a _DSM, I think they have to support function 0.
> > 
> > They should, but didn't.  Kernel v4.6 works with qemu 2.6, kernel v4.7
> > does not, so the kernel needs to be fixed.
> > 
> 
> Sorry.
> 
> I do not know why you guys think QEMU does not support function 0. It
> already returns 0 to indicates "no functions are supported
> (other than function zero)".
> 

  The currently upstream version of acpi_check_dsm doesn't handle this
  case,  it assumes at least one function other than zero is also being
  returned (as it assumes bit 0 is set.)

  Instead of the proposed change we should have something like:

index 22c0995..3ecf462 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -702,6 +702,9 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs)
        if ((mask & 0x1) && (mask & funcs) == funcs)
                return true;
 
+       if ((mask == 0) && (funcs == 1))
+               return true;
+
        return false;
 }
 EXPORT_SYMBOL(acpi_check_dsm);


> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm