Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On 6/8/2018 8:04 PM, Darren Hart wrote:
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
>> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes
>> wrote:
>>> If the WSMT ACPI table is present and indicates that a fixed communication
>>> buffer should be used, use the firmware-specified buffer instead of
>>> allocating a buffer in memory for communications between the dcdbas driver
>>> and firmare.
>>
>>> config DCDBAS
>>> tristate "Dell Systems Management Base Driver"
>>> - depends on X86
>>> + depends on X86 && ACPI
>
>
>>
>> Hmm... I'm not sure about this dependency.
>> So, the question is do all users actually need this? How did it work
>> previously? How has this been tested in case when command line has
>> "acpi=off" (yes, this one just for sake of test, I don't believe it's
>> a real use case)?
>
> Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
> driver which intersected with this one this needs to be thoroughly
> covered, tested, and thought through. Linus was generous in the
> number of attempts it took us to get that right.
>
> Did DCDBAS ever work on a system without ACPI?
>
It appears to compile ok without ACPI enabled... looks like acpi_get_table
just returns a constant when CONFIG_ACPI isn't there, which makes all the WSMT
stuff get optimized out. So I don't guess we even need an "#ifdef CONFIG_ACPI".
>>
>>> #include
>>> #include
>>> #include
>>> +#include
>>
>> Please, try to keep an order as much as possible.
>> For example, in given context this line should be before string.h (I
>> didn't check the actual file, perhaps even upper).
>>
>>> #include
>>>
>>> #include "dcdbas.h"
>>
>>> /* Calling Interface SMI */
>>> - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
>>> + smi_cmd->ebx = smi_data_buf_phys_addr
>>> + + offsetof(struct smi_cmd, command_buffer);
>>
>> Please, keep at least + on the previous line.
>> Also, I'm not sure what is the difference now. Especially for previous
>> users when this wasn't the part of the driver.
>> Some explanation needed.
>>
I'll fix this.
>>> +static u8 checksum(u8 *buffer, u8 length)
>>> +{
>>> + u8 sum = 0;
>>> + u8 *end = buffer + length;
>>> +
>>> + while (buffer < end)
>>> + sum = (u8)(sum + *(buffer++));
>>
>> Why not simple
>>
>> sum += *buffer++;
>>
>> ?
>>
>>> + return sum;
>>> +}
>>
>> And I would rather check if we have similar algoritms already in the
>> kernel which we might re-use.
>
> Seems to be some options in lib/checksum.c to check.
>
I couldn't find anything in checksum.c or elsewhere that I could just
include that would do a byte checksum, not a word. I copied this code
from acpi_tb_checksum (in drivers/acpi/acpica/tbprint.c), but I can
shorten it as suggested.
>>
>>> +
>>> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
>>> +{
>>> + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
>>> +
>>
>>> + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>>
>> I'm not sure about strings operation here.
>> I would rather do like with other magic constants: introduce hex value
>> and compare it as unsigned integer.
>>
>> Also, it might be a warning, since \0 wasn't ever checked from the
>> string literal. Though, I'm not sure if it applicable to strncmp()
>> function (it's for strncpy for sure).
>
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
>
>>
>>> + return NULL;
>>> +
>>> + if (checksum(addr, eps->length) != 0)
>>> + return NULL;
>>> +
>>> + return eps;
>>> +}
>>> +
>>> +static int dcdbas_check_wsmt(void)
>>> +{
>>> + struct acpi_table_wsmt *wsmt = NULL;
>>> + struct smm_eps_table *eps = NULL;
>>> + u8 *addr;
>>> +
>>> + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header
>>> **));
>>> + if (!wsmt)
>>> + return 0;
>>> +
>>> + /* Check if WSMT ACPI table shows that protection is enabled */
>>> + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
>>> + || !(wsmt->protection_flags
>>> +& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
>>> + return 0;
>>> +
>>> + /* Scan for EPS (entry point structure) */
>>> + for (addr = (u8 *)__va(0xf);
>>> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) &&
>>> !eps;
>>
>> Perhaps better to do
>>
>> for (...) {
>> eps = ...();
>> if (eps)
>> break;
>> }
>>
>> Also I've a feeling that 0xf constant is defined already somewhere
>> under arch/x86/include/asm or evem include/linux.
>
> But... is it defined for this purpose?
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On 6/8/2018 8:04 PM, Darren Hart wrote:
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
>> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes
>> wrote:
>>> If the WSMT ACPI table is present and indicates that a fixed communication
>>> buffer should be used, use the firmware-specified buffer instead of
>>> allocating a buffer in memory for communications between the dcdbas driver
>>> and firmare.
>>
>>> config DCDBAS
>>> tristate "Dell Systems Management Base Driver"
>>> - depends on X86
>>> + depends on X86 && ACPI
>
>
>>
>> Hmm... I'm not sure about this dependency.
>> So, the question is do all users actually need this? How did it work
>> previously? How has this been tested in case when command line has
>> "acpi=off" (yes, this one just for sake of test, I don't believe it's
>> a real use case)?
>
> Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
> driver which intersected with this one this needs to be thoroughly
> covered, tested, and thought through. Linus was generous in the
> number of attempts it took us to get that right.
>
> Did DCDBAS ever work on a system without ACPI?
>
It appears to compile ok without ACPI enabled... looks like acpi_get_table
just returns a constant when CONFIG_ACPI isn't there, which makes all the WSMT
stuff get optimized out. So I don't guess we even need an "#ifdef CONFIG_ACPI".
>>
>>> #include
>>> #include
>>> #include
>>> +#include
>>
>> Please, try to keep an order as much as possible.
>> For example, in given context this line should be before string.h (I
>> didn't check the actual file, perhaps even upper).
>>
>>> #include
>>>
>>> #include "dcdbas.h"
>>
>>> /* Calling Interface SMI */
>>> - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
>>> + smi_cmd->ebx = smi_data_buf_phys_addr
>>> + + offsetof(struct smi_cmd, command_buffer);
>>
>> Please, keep at least + on the previous line.
>> Also, I'm not sure what is the difference now. Especially for previous
>> users when this wasn't the part of the driver.
>> Some explanation needed.
>>
I'll fix this.
>>> +static u8 checksum(u8 *buffer, u8 length)
>>> +{
>>> + u8 sum = 0;
>>> + u8 *end = buffer + length;
>>> +
>>> + while (buffer < end)
>>> + sum = (u8)(sum + *(buffer++));
>>
>> Why not simple
>>
>> sum += *buffer++;
>>
>> ?
>>
>>> + return sum;
>>> +}
>>
>> And I would rather check if we have similar algoritms already in the
>> kernel which we might re-use.
>
> Seems to be some options in lib/checksum.c to check.
>
I couldn't find anything in checksum.c or elsewhere that I could just
include that would do a byte checksum, not a word. I copied this code
from acpi_tb_checksum (in drivers/acpi/acpica/tbprint.c), but I can
shorten it as suggested.
>>
>>> +
>>> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
>>> +{
>>> + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
>>> +
>>
>>> + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>>
>> I'm not sure about strings operation here.
>> I would rather do like with other magic constants: introduce hex value
>> and compare it as unsigned integer.
>>
>> Also, it might be a warning, since \0 wasn't ever checked from the
>> string literal. Though, I'm not sure if it applicable to strncmp()
>> function (it's for strncpy for sure).
>
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
>
>>
>>> + return NULL;
>>> +
>>> + if (checksum(addr, eps->length) != 0)
>>> + return NULL;
>>> +
>>> + return eps;
>>> +}
>>> +
>>> +static int dcdbas_check_wsmt(void)
>>> +{
>>> + struct acpi_table_wsmt *wsmt = NULL;
>>> + struct smm_eps_table *eps = NULL;
>>> + u8 *addr;
>>> +
>>> + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header
>>> **));
>>> + if (!wsmt)
>>> + return 0;
>>> +
>>> + /* Check if WSMT ACPI table shows that protection is enabled */
>>> + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
>>> + || !(wsmt->protection_flags
>>> +& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
>>> + return 0;
>>> +
>>> + /* Scan for EPS (entry point structure) */
>>> + for (addr = (u8 *)__va(0xf);
>>> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) &&
>>> !eps;
>>
>> Perhaps better to do
>>
>> for (...) {
>> eps = ...();
>> if (eps)
>> break;
>> }
>>
>> Also I've a feeling that 0xf constant is defined already somewhere
>> under arch/x86/include/asm or evem include/linux.
>
> But... is it defined for this purpose?
RE: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> -Original Message-
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Friday, June 8, 2018 8:04 PM
> To: Andy Shevchenko
> Cc: Stuart Hayes; Warzecha, Douglas; Limonciello, Mario; Dominguez, Jared;
> Linux
> Kernel Mailing List; Platform Driver
> Subject: Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
>
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes
> wrote:
> > > If the WSMT ACPI table is present and indicates that a fixed communication
> > > buffer should be used, use the firmware-specified buffer instead of
> > > allocating a buffer in memory for communications between the dcdbas driver
> > > and firmare.
> >
> > > config DCDBAS
> > > tristate "Dell Systems Management Base Driver"
> > > - depends on X86
> > > + depends on X86 && ACPI
>
>
> >
> > Hmm... I'm not sure about this dependency.
> > So, the question is do all users actually need this? How did it work
> > previously? How has this been tested in case when command line has
> > "acpi=off" (yes, this one just for sake of test, I don't believe it's
> > a real use case)?
>
> Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
> driver which intersected with this one this needs to be thoroughly
> covered, tested, and thought through. Linus was generous in the
> number of attempts it took us to get that right.
>
> Did DCDBAS ever work on a system without ACPI?
Yes, I would expect it would have functioned on a system with kernel's
ACPI disabled. However I also agree this isn't a real use case with a modern
Machine.
Perhaps the right thing to do is
#ifdef CONFIG_ACPI
For the WSMT relevant items in this patch?
>
> >
> > > #include
> > > #include
> > > #include
> > > +#include
> >
> > Please, try to keep an order as much as possible.
> > For example, in given context this line should be before string.h (I
> > didn't check the actual file, perhaps even upper).
> >
> > > #include
> > >
> > > #include "dcdbas.h"
> >
> > > /* Calling Interface SMI */
> > > - smi_cmd->ebx = (u32)
> > > virt_to_phys(smi_cmd->command_buffer);
> > > + smi_cmd->ebx = smi_data_buf_phys_addr
> > > + + offsetof(struct smi_cmd,
> > > command_buffer);
> >
> > Please, keep at least + on the previous line.
> > Also, I'm not sure what is the difference now. Especially for previous
> > users when this wasn't the part of the driver.
> > Some explanation needed.
> >
> > > +static u8 checksum(u8 *buffer, u8 length)
> > > +{
> > > + u8 sum = 0;
> > > + u8 *end = buffer + length;
> > > +
> > > + while (buffer < end)
> > > + sum = (u8)(sum + *(buffer++));
> >
> > Why not simple
> >
> > sum += *buffer++;
> >
> > ?
> >
> > > + return sum;
> > > +}
> >
> > And I would rather check if we have similar algoritms already in the
> > kernel which we might re-use.
>
> Seems to be some options in lib/checksum.c to check.
>
> >
> > > +
> > > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > > +{
> > > + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > > +
> >
> > > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> >
> > I'm not sure about strings operation here.
> > I would rather do like with other magic constants: introduce hex value
> > and compare it as unsigned integer.
> >
> > Also, it might be a warning, since \0 wasn't ever checked from the
> > string literal. Though, I'm not sure if it applicable to strncmp()
> > function (it's for strncpy for sure).
>
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
>
> >
> > > + return NULL;
> > > +
> > > + if (checksum(addr, eps->length) != 0)
> > > + return NULL;
> > > +
> > > + return eps;
> > > +}
> > > +
> > > +static int dcdbas_check_wsmt(void)
>
RE: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> -Original Message-
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Friday, June 8, 2018 8:04 PM
> To: Andy Shevchenko
> Cc: Stuart Hayes; Warzecha, Douglas; Limonciello, Mario; Dominguez, Jared;
> Linux
> Kernel Mailing List; Platform Driver
> Subject: Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
>
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes
> wrote:
> > > If the WSMT ACPI table is present and indicates that a fixed communication
> > > buffer should be used, use the firmware-specified buffer instead of
> > > allocating a buffer in memory for communications between the dcdbas driver
> > > and firmare.
> >
> > > config DCDBAS
> > > tristate "Dell Systems Management Base Driver"
> > > - depends on X86
> > > + depends on X86 && ACPI
>
>
> >
> > Hmm... I'm not sure about this dependency.
> > So, the question is do all users actually need this? How did it work
> > previously? How has this been tested in case when command line has
> > "acpi=off" (yes, this one just for sake of test, I don't believe it's
> > a real use case)?
>
> Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
> driver which intersected with this one this needs to be thoroughly
> covered, tested, and thought through. Linus was generous in the
> number of attempts it took us to get that right.
>
> Did DCDBAS ever work on a system without ACPI?
Yes, I would expect it would have functioned on a system with kernel's
ACPI disabled. However I also agree this isn't a real use case with a modern
Machine.
Perhaps the right thing to do is
#ifdef CONFIG_ACPI
For the WSMT relevant items in this patch?
>
> >
> > > #include
> > > #include
> > > #include
> > > +#include
> >
> > Please, try to keep an order as much as possible.
> > For example, in given context this line should be before string.h (I
> > didn't check the actual file, perhaps even upper).
> >
> > > #include
> > >
> > > #include "dcdbas.h"
> >
> > > /* Calling Interface SMI */
> > > - smi_cmd->ebx = (u32)
> > > virt_to_phys(smi_cmd->command_buffer);
> > > + smi_cmd->ebx = smi_data_buf_phys_addr
> > > + + offsetof(struct smi_cmd,
> > > command_buffer);
> >
> > Please, keep at least + on the previous line.
> > Also, I'm not sure what is the difference now. Especially for previous
> > users when this wasn't the part of the driver.
> > Some explanation needed.
> >
> > > +static u8 checksum(u8 *buffer, u8 length)
> > > +{
> > > + u8 sum = 0;
> > > + u8 *end = buffer + length;
> > > +
> > > + while (buffer < end)
> > > + sum = (u8)(sum + *(buffer++));
> >
> > Why not simple
> >
> > sum += *buffer++;
> >
> > ?
> >
> > > + return sum;
> > > +}
> >
> > And I would rather check if we have similar algoritms already in the
> > kernel which we might re-use.
>
> Seems to be some options in lib/checksum.c to check.
>
> >
> > > +
> > > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > > +{
> > > + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > > +
> >
> > > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> >
> > I'm not sure about strings operation here.
> > I would rather do like with other magic constants: introduce hex value
> > and compare it as unsigned integer.
> >
> > Also, it might be a warning, since \0 wasn't ever checked from the
> > string literal. Though, I'm not sure if it applicable to strncmp()
> > function (it's for strncpy for sure).
>
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
>
> >
> > > + return NULL;
> > > +
> > > + if (checksum(addr, eps->length) != 0)
> > > + return NULL;
> > > +
> > > + return eps;
> > > +}
> > > +
> > > +static int dcdbas_check_wsmt(void)
>
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On Sat, Jun 9, 2018 at 4:04 AM, Darren Hart wrote:
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
>> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes
>> wrote:
>> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
>> > +{
>> > + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
>> > +
>>
>> > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>>
>> I'm not sure about strings operation here.
>> I would rather do like with other magic constants: introduce hex value
>> and compare it as unsigned integer.
>>
>> Also, it might be a warning, since \0 wasn't ever checked from the
>> string literal. Though, I'm not sure if it applicable to strncmp()
>> function (it's for strncpy for sure).
>
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
Yeah, consistency is usually a priority over style. Though in the
context below I see also magic numbers instead of strings.
I have no strong opinion here which one is better to follow.
>> Also I've a feeling that 0xf constant is defined already somewhere
>> under arch/x86/include/asm or evem include/linux.
>
> But... is it defined for this purpose? If not, I'd prefer it hardcoded
> (or with a DEFINE).
OK, I did a research, and the following is the result.
arch/x86/kernel/mpparse.c:636: smp_scan_config(0xF, 0x1))
arch/x86/kernel/probe_roms.c:27:.start = 0xf,
arch/x86/pci/irq.c:91: * Search 0xf -- 0xf for the PCI IRQ
Routing Table.
arch/x86/pci/irq.c:105: for (addr = (u8 *) __va(0xf); addr < (u8
*) __va(0x10); addr += 16) {
arch/x86/platform/geode/alix.c:34:#define BIOS_SIGNATURE_TINYBIOS
0xf
arch/x86/platform/olpc/olpc-xo1-pm.c:36:} ofw_bios_entry = { 0xF +
PAGE_OFFSET, __KERNEL_CS };
arch/x86/platform/ts5500/ts5500.c:103: bios = ioremap(0xf, 0x1);
Perhaps it would be a good idea to introduce such constant in the future.
Note mpparse.c and irq.c, that's similar purpose of the address range.
>> > #define SMI_CMD_MAGIC (0x534D4931)
>> >
>> > +#define SMM_EPS_SIG"$SCB"
>>
>> Just integer like above and put the sting as a comment.
>> (Side note: above magic also looks like string)
>
> Given the above, I think we can use the more recognizable string - since
> that is clearly how they think of this label.
OK with me!
--
With Best Regards,
Andy Shevchenko
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On Sat, Jun 9, 2018 at 4:04 AM, Darren Hart wrote:
> On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
>> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes
>> wrote:
>> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
>> > +{
>> > + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
>> > +
>>
>> > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>>
>> I'm not sure about strings operation here.
>> I would rather do like with other magic constants: introduce hex value
>> and compare it as unsigned integer.
>>
>> Also, it might be a warning, since \0 wasn't ever checked from the
>> string literal. Though, I'm not sure if it applicable to strncmp()
>> function (it's for strncpy for sure).
>
> I think we're OK here, and we're being consistent with the
> dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
> one or something else, but doing it in HEX ended up being more
> confusing. The \0 isn't an issue since strncmp will only compare the n
> (4) bytes.
Yeah, consistency is usually a priority over style. Though in the
context below I see also magic numbers instead of strings.
I have no strong opinion here which one is better to follow.
>> Also I've a feeling that 0xf constant is defined already somewhere
>> under arch/x86/include/asm or evem include/linux.
>
> But... is it defined for this purpose? If not, I'd prefer it hardcoded
> (or with a DEFINE).
OK, I did a research, and the following is the result.
arch/x86/kernel/mpparse.c:636: smp_scan_config(0xF, 0x1))
arch/x86/kernel/probe_roms.c:27:.start = 0xf,
arch/x86/pci/irq.c:91: * Search 0xf -- 0xf for the PCI IRQ
Routing Table.
arch/x86/pci/irq.c:105: for (addr = (u8 *) __va(0xf); addr < (u8
*) __va(0x10); addr += 16) {
arch/x86/platform/geode/alix.c:34:#define BIOS_SIGNATURE_TINYBIOS
0xf
arch/x86/platform/olpc/olpc-xo1-pm.c:36:} ofw_bios_entry = { 0xF +
PAGE_OFFSET, __KERNEL_CS };
arch/x86/platform/ts5500/ts5500.c:103: bios = ioremap(0xf, 0x1);
Perhaps it would be a good idea to introduce such constant in the future.
Note mpparse.c and irq.c, that's similar purpose of the address range.
>> > #define SMI_CMD_MAGIC (0x534D4931)
>> >
>> > +#define SMM_EPS_SIG"$SCB"
>>
>> Just integer like above and put the sting as a comment.
>> (Side note: above magic also looks like string)
>
> Given the above, I think we can use the more recognizable string - since
> that is clearly how they think of this label.
OK with me!
--
With Best Regards,
Andy Shevchenko
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes wrote:
> > If the WSMT ACPI table is present and indicates that a fixed communication
> > buffer should be used, use the firmware-specified buffer instead of
> > allocating a buffer in memory for communications between the dcdbas driver
> > and firmare.
>
> > config DCDBAS
> > tristate "Dell Systems Management Base Driver"
> > - depends on X86
> > + depends on X86 && ACPI
>
> Hmm... I'm not sure about this dependency.
> So, the question is do all users actually need this? How did it work
> previously? How has this been tested in case when command line has
> "acpi=off" (yes, this one just for sake of test, I don't believe it's
> a real use case)?
Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
driver which intersected with this one this needs to be thoroughly
covered, tested, and thought through. Linus was generous in the
number of attempts it took us to get that right.
Did DCDBAS ever work on a system without ACPI?
>
> > #include
> > #include
> > #include
> > +#include
>
> Please, try to keep an order as much as possible.
> For example, in given context this line should be before string.h (I
> didn't check the actual file, perhaps even upper).
>
> > #include
> >
> > #include "dcdbas.h"
>
> > /* Calling Interface SMI */
> > - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> > + smi_cmd->ebx = smi_data_buf_phys_addr
> > + + offsetof(struct smi_cmd, command_buffer);
>
> Please, keep at least + on the previous line.
> Also, I'm not sure what is the difference now. Especially for previous
> users when this wasn't the part of the driver.
> Some explanation needed.
>
> > +static u8 checksum(u8 *buffer, u8 length)
> > +{
> > + u8 sum = 0;
> > + u8 *end = buffer + length;
> > +
> > + while (buffer < end)
> > + sum = (u8)(sum + *(buffer++));
>
> Why not simple
>
> sum += *buffer++;
>
> ?
>
> > + return sum;
> > +}
>
> And I would rather check if we have similar algoritms already in the
> kernel which we might re-use.
Seems to be some options in lib/checksum.c to check.
>
> > +
> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > +{
> > + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > +
>
> > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>
> I'm not sure about strings operation here.
> I would rather do like with other magic constants: introduce hex value
> and compare it as unsigned integer.
>
> Also, it might be a warning, since \0 wasn't ever checked from the
> string literal. Though, I'm not sure if it applicable to strncmp()
> function (it's for strncpy for sure).
I think we're OK here, and we're being consistent with the
dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
one or something else, but doing it in HEX ended up being more
confusing. The \0 isn't an issue since strncmp will only compare the n
(4) bytes.
>
> > + return NULL;
> > +
> > + if (checksum(addr, eps->length) != 0)
> > + return NULL;
> > +
> > + return eps;
> > +}
> > +
> > +static int dcdbas_check_wsmt(void)
> > +{
> > + struct acpi_table_wsmt *wsmt = NULL;
> > + struct smm_eps_table *eps = NULL;
> > + u8 *addr;
> > +
> > + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header
> > **));
> > + if (!wsmt)
> > + return 0;
> > +
> > + /* Check if WSMT ACPI table shows that protection is enabled */
> > + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> > + || !(wsmt->protection_flags
> > +& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> > + return 0;
> > +
> > + /* Scan for EPS (entry point structure) */
> > + for (addr = (u8 *)__va(0xf);
> > +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) &&
> > !eps;
>
> Perhaps better to do
>
> for (...) {
> eps = ...();
> if (eps)
> break;
> }
>
> Also I've a feeling that 0xf constant is defined already somewhere
> under arch/x86/include/asm or evem include/linux.
But... is it defined for this purpose? If not, I'd prefer it hardcoded
(or with a DEFINE).
>
> > +addr += 1)
>
> += 1?!
> All tables I saw in BIOS are aligned with 16 bytes. Is it the case here?
>
> Is there any other means to check if the table present? ACPI code?
> Method / variable?
>
> > + eps = check_eps_table(addr);
> > +
> > + if (!eps) {
> > + dev_dbg(_pdev->dev, "found WSMT, but no EPS
> > found\n");
> > + return -ENODEV;
> > + }
> > +
> > + /*
> > +* Get physical address of buffer and map to virtual address.
> > +* Table gives size in 4K
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On Thu, Jun 07, 2018 at 08:11:41PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes wrote:
> > If the WSMT ACPI table is present and indicates that a fixed communication
> > buffer should be used, use the firmware-specified buffer instead of
> > allocating a buffer in memory for communications between the dcdbas driver
> > and firmare.
>
> > config DCDBAS
> > tristate "Dell Systems Management Base Driver"
> > - depends on X86
> > + depends on X86 && ACPI
>
> Hmm... I'm not sure about this dependency.
> So, the question is do all users actually need this? How did it work
> previously? How has this been tested in case when command line has
> "acpi=off" (yes, this one just for sake of test, I don't believe it's
> a real use case)?
Yeah... after the 4.16 and 4.17 KConfig fumbling around the SMBIOS
driver which intersected with this one this needs to be thoroughly
covered, tested, and thought through. Linus was generous in the
number of attempts it took us to get that right.
Did DCDBAS ever work on a system without ACPI?
>
> > #include
> > #include
> > #include
> > +#include
>
> Please, try to keep an order as much as possible.
> For example, in given context this line should be before string.h (I
> didn't check the actual file, perhaps even upper).
>
> > #include
> >
> > #include "dcdbas.h"
>
> > /* Calling Interface SMI */
> > - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> > + smi_cmd->ebx = smi_data_buf_phys_addr
> > + + offsetof(struct smi_cmd, command_buffer);
>
> Please, keep at least + on the previous line.
> Also, I'm not sure what is the difference now. Especially for previous
> users when this wasn't the part of the driver.
> Some explanation needed.
>
> > +static u8 checksum(u8 *buffer, u8 length)
> > +{
> > + u8 sum = 0;
> > + u8 *end = buffer + length;
> > +
> > + while (buffer < end)
> > + sum = (u8)(sum + *(buffer++));
>
> Why not simple
>
> sum += *buffer++;
>
> ?
>
> > + return sum;
> > +}
>
> And I would rather check if we have similar algoritms already in the
> kernel which we might re-use.
Seems to be some options in lib/checksum.c to check.
>
> > +
> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > +{
> > + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > +
>
> > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
>
> I'm not sure about strings operation here.
> I would rather do like with other magic constants: introduce hex value
> and compare it as unsigned integer.
>
> Also, it might be a warning, since \0 wasn't ever checked from the
> string literal. Though, I'm not sure if it applicable to strncmp()
> function (it's for strncpy for sure).
I think we're OK here, and we're being consistent with the
dell-wmi-descriptor test for "DELL WMI". I don't recall if it was that
one or something else, but doing it in HEX ended up being more
confusing. The \0 isn't an issue since strncmp will only compare the n
(4) bytes.
>
> > + return NULL;
> > +
> > + if (checksum(addr, eps->length) != 0)
> > + return NULL;
> > +
> > + return eps;
> > +}
> > +
> > +static int dcdbas_check_wsmt(void)
> > +{
> > + struct acpi_table_wsmt *wsmt = NULL;
> > + struct smm_eps_table *eps = NULL;
> > + u8 *addr;
> > +
> > + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header
> > **));
> > + if (!wsmt)
> > + return 0;
> > +
> > + /* Check if WSMT ACPI table shows that protection is enabled */
> > + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> > + || !(wsmt->protection_flags
> > +& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> > + return 0;
> > +
> > + /* Scan for EPS (entry point structure) */
> > + for (addr = (u8 *)__va(0xf);
> > +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) &&
> > !eps;
>
> Perhaps better to do
>
> for (...) {
> eps = ...();
> if (eps)
> break;
> }
>
> Also I've a feeling that 0xf constant is defined already somewhere
> under arch/x86/include/asm or evem include/linux.
But... is it defined for this purpose? If not, I'd prefer it hardcoded
(or with a DEFINE).
>
> > +addr += 1)
>
> += 1?!
> All tables I saw in BIOS are aligned with 16 bytes. Is it the case here?
>
> Is there any other means to check if the table present? ACPI code?
> Method / variable?
>
> > + eps = check_eps_table(addr);
> > +
> > + if (!eps) {
> > + dev_dbg(_pdev->dev, "found WSMT, but no EPS
> > found\n");
> > + return -ENODEV;
> > + }
> > +
> > + /*
> > +* Get physical address of buffer and map to virtual address.
> > +* Table gives size in 4K
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On Thu, Jun 07, 2018 at 04:07:50PM +, mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Stuart Hayes [mailto:stuart.w.ha...@gmail.com]
> > Sent: Thursday, June 7, 2018 11:00 AM
> > To: Warzecha, Douglas
> > Cc: Limonciello, Mario; Dominguez, Jared; linux-kernel@vger.kernel.org;
> > platform-
> > driver-...@vger.kernel.org
> > Subject: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> >
> > If the WSMT ACPI table is present and indicates that a fixed communication
> > buffer should be used, use the firmware-specified buffer instead of
> > allocating a buffer in memory for communications between the dcdbas driver
> > and firmare.
> >
> > Signed-off-by: Stuart Hayes
> > Tested-by: Mario Limonciello
> > Acked-by: Doug Warzecha
> > ---
> > v2 Bumped driver version to 5.6.0-3.3
> >
> >
> > drivers/firmware/Kconfig | 2 +-
> > drivers/firmware/dcdbas.c | 102
> > +++---
> > drivers/firmware/dcdbas.h | 11 +
> > 3 files changed, 109 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index b7c748248e53..a2bd6092bfa1 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -125,7 +125,7 @@ config DELL_RBU
> >
> > config DCDBAS
> > tristate "Dell Systems Management Base Driver"
> > - depends on X86
> > + depends on X86 && ACPI
> > help
> > The Dell Systems Management Base Driver provides a sysfs interface
> > for systems management software to perform System Management
> > diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> > index 0bdea60c65dd..1699cfdcefe4 100644
> > --- a/drivers/firmware/dcdbas.c
> > +++ b/drivers/firmware/dcdbas.c
> > @@ -36,12 +36,13 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "dcdbas.h"
> >
> > #define DRIVER_NAME"dcdbas"
> > -#define DRIVER_VERSION "5.6.0-3.2"
> > +#define DRIVER_VERSION "5.6.0-3.3"
> > #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
> >
> > static struct platform_device *dcdbas_pdev;
> > @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
> > static u8 *smi_data_buf;
> > static dma_addr_t smi_data_buf_handle;
> > static unsigned long smi_data_buf_size;
> > +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
> > static u32 smi_data_buf_phys_addr;
> > static DEFINE_MUTEX(smi_data_lock);
> > +static u8 *eps_buffer;
> >
> > static unsigned int host_control_action;
> > static unsigned int host_control_smi_type;
> > static unsigned int host_control_on_shutdown;
> >
> > +static bool wsmt_enabled;
> > +
> > /**
> > * smi_data_buf_free: free SMI data buffer
> > */
> > static void smi_data_buf_free(void)
> > {
> > - if (!smi_data_buf)
> > + if (!smi_data_buf || wsmt_enabled)
> > return;
> >
> > dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
> > @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
> > if (smi_data_buf_size >= size)
> > return 0;
> >
> > - if (size > MAX_SMI_DATA_BUF_SIZE)
> > + if (size > max_smi_data_buf_size)
> > return -EINVAL;
> >
> > /* new buffer is needed */
> > @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct
> > kobject
> > *kobj,
> > {
> > ssize_t ret;
> >
> > - if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> > + if ((pos + count) > max_smi_data_buf_size)
> > return -EINVAL;
> >
> > mutex_lock(_data_lock);
> > @@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
> > break;
> > case 1:
> > /* Calling Interface SMI */
> > - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> > + smi_cmd->ebx = smi_data_buf_phys_addr
> > + + offsetof(struct smi_cmd, command_buffer);
> > ret = dcdbas_smi_request(smi_cmd);
> > if (!ret)
> > ret = count;
> > @@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
> > }
> > }
> >
> > +/* WSMT */
> > +
> > +static u8 checksum(u8 *buffer, u8 length)
> > +{
> > + u8 sum = 0;
> > + u8 *end = buffer + length;
> > +
> > + while (buffer < end)
> > + sum = (u8)(sum + *(buffer++));
> > + return sum;
> > +}
> > +
> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > +{
> > + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > +
> > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> > + return NULL;
> > +
> > + if (checksum(addr, eps->length) != 0)
> > + return NULL;
> > +
> > + return eps;
> > +}
> > +
> > +static int dcdbas_check_wsmt(void)
> > +{
> > + struct acpi_table_wsmt *wsmt = NULL;
> > + struct smm_eps_table *eps = NULL;
> > + u8 *addr;
> > +
> > + acpi_get_table(ACPI_SIG_WSMT, 0,
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On Thu, Jun 07, 2018 at 04:07:50PM +, mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Stuart Hayes [mailto:stuart.w.ha...@gmail.com]
> > Sent: Thursday, June 7, 2018 11:00 AM
> > To: Warzecha, Douglas
> > Cc: Limonciello, Mario; Dominguez, Jared; linux-kernel@vger.kernel.org;
> > platform-
> > driver-...@vger.kernel.org
> > Subject: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> >
> > If the WSMT ACPI table is present and indicates that a fixed communication
> > buffer should be used, use the firmware-specified buffer instead of
> > allocating a buffer in memory for communications between the dcdbas driver
> > and firmare.
> >
> > Signed-off-by: Stuart Hayes
> > Tested-by: Mario Limonciello
> > Acked-by: Doug Warzecha
> > ---
> > v2 Bumped driver version to 5.6.0-3.3
> >
> >
> > drivers/firmware/Kconfig | 2 +-
> > drivers/firmware/dcdbas.c | 102
> > +++---
> > drivers/firmware/dcdbas.h | 11 +
> > 3 files changed, 109 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index b7c748248e53..a2bd6092bfa1 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -125,7 +125,7 @@ config DELL_RBU
> >
> > config DCDBAS
> > tristate "Dell Systems Management Base Driver"
> > - depends on X86
> > + depends on X86 && ACPI
> > help
> > The Dell Systems Management Base Driver provides a sysfs interface
> > for systems management software to perform System Management
> > diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> > index 0bdea60c65dd..1699cfdcefe4 100644
> > --- a/drivers/firmware/dcdbas.c
> > +++ b/drivers/firmware/dcdbas.c
> > @@ -36,12 +36,13 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "dcdbas.h"
> >
> > #define DRIVER_NAME"dcdbas"
> > -#define DRIVER_VERSION "5.6.0-3.2"
> > +#define DRIVER_VERSION "5.6.0-3.3"
> > #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
> >
> > static struct platform_device *dcdbas_pdev;
> > @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
> > static u8 *smi_data_buf;
> > static dma_addr_t smi_data_buf_handle;
> > static unsigned long smi_data_buf_size;
> > +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
> > static u32 smi_data_buf_phys_addr;
> > static DEFINE_MUTEX(smi_data_lock);
> > +static u8 *eps_buffer;
> >
> > static unsigned int host_control_action;
> > static unsigned int host_control_smi_type;
> > static unsigned int host_control_on_shutdown;
> >
> > +static bool wsmt_enabled;
> > +
> > /**
> > * smi_data_buf_free: free SMI data buffer
> > */
> > static void smi_data_buf_free(void)
> > {
> > - if (!smi_data_buf)
> > + if (!smi_data_buf || wsmt_enabled)
> > return;
> >
> > dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
> > @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
> > if (smi_data_buf_size >= size)
> > return 0;
> >
> > - if (size > MAX_SMI_DATA_BUF_SIZE)
> > + if (size > max_smi_data_buf_size)
> > return -EINVAL;
> >
> > /* new buffer is needed */
> > @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct
> > kobject
> > *kobj,
> > {
> > ssize_t ret;
> >
> > - if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> > + if ((pos + count) > max_smi_data_buf_size)
> > return -EINVAL;
> >
> > mutex_lock(_data_lock);
> > @@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
> > break;
> > case 1:
> > /* Calling Interface SMI */
> > - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> > + smi_cmd->ebx = smi_data_buf_phys_addr
> > + + offsetof(struct smi_cmd, command_buffer);
> > ret = dcdbas_smi_request(smi_cmd);
> > if (!ret)
> > ret = count;
> > @@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
> > }
> > }
> >
> > +/* WSMT */
> > +
> > +static u8 checksum(u8 *buffer, u8 length)
> > +{
> > + u8 sum = 0;
> > + u8 *end = buffer + length;
> > +
> > + while (buffer < end)
> > + sum = (u8)(sum + *(buffer++));
> > + return sum;
> > +}
> > +
> > +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> > +{
> > + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> > +
> > + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> > + return NULL;
> > +
> > + if (checksum(addr, eps->length) != 0)
> > + return NULL;
> > +
> > + return eps;
> > +}
> > +
> > +static int dcdbas_check_wsmt(void)
> > +{
> > + struct acpi_table_wsmt *wsmt = NULL;
> > + struct smm_eps_table *eps = NULL;
> > + u8 *addr;
> > +
> > + acpi_get_table(ACPI_SIG_WSMT, 0,
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes wrote:
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.
> config DCDBAS
> tristate "Dell Systems Management Base Driver"
> - depends on X86
> + depends on X86 && ACPI
Hmm... I'm not sure about this dependency.
So, the question is do all users actually need this? How did it work
previously? How has this been tested in case when command line has
"acpi=off" (yes, this one just for sake of test, I don't believe it's
a real use case)?
> #include
> #include
> #include
> +#include
Please, try to keep an order as much as possible.
For example, in given context this line should be before string.h (I
didn't check the actual file, perhaps even upper).
> #include
>
> #include "dcdbas.h"
> /* Calling Interface SMI */
> - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> + smi_cmd->ebx = smi_data_buf_phys_addr
> + + offsetof(struct smi_cmd, command_buffer);
Please, keep at least + on the previous line.
Also, I'm not sure what is the difference now. Especially for previous
users when this wasn't the part of the driver.
Some explanation needed.
> +static u8 checksum(u8 *buffer, u8 length)
> +{
> + u8 sum = 0;
> + u8 *end = buffer + length;
> +
> + while (buffer < end)
> + sum = (u8)(sum + *(buffer++));
Why not simple
sum += *buffer++;
?
> + return sum;
> +}
And I would rather check if we have similar algoritms already in the
kernel which we might re-use.
> +
> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> +{
> + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> +
> + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
I'm not sure about strings operation here.
I would rather do like with other magic constants: introduce hex value
and compare it as unsigned integer.
Also, it might be a warning, since \0 wasn't ever checked from the
string literal. Though, I'm not sure if it applicable to strncmp()
function (it's for strncpy for sure).
> + return NULL;
> +
> + if (checksum(addr, eps->length) != 0)
> + return NULL;
> +
> + return eps;
> +}
> +
> +static int dcdbas_check_wsmt(void)
> +{
> + struct acpi_table_wsmt *wsmt = NULL;
> + struct smm_eps_table *eps = NULL;
> + u8 *addr;
> +
> + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
> + if (!wsmt)
> + return 0;
> +
> + /* Check if WSMT ACPI table shows that protection is enabled */
> + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> + || !(wsmt->protection_flags
> +& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> + return 0;
> +
> + /* Scan for EPS (entry point structure) */
> + for (addr = (u8 *)__va(0xf);
> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) &&
> !eps;
Perhaps better to do
for (...) {
eps = ...();
if (eps)
break;
}
Also I've a feeling that 0xf constant is defined already somewhere
under arch/x86/include/asm or evem include/linux.
> +addr += 1)
+= 1?!
All tables I saw in BIOS are aligned with 16 bytes. Is it the case here?
Is there any other means to check if the table present? ACPI code?
Method / variable?
> + eps = check_eps_table(addr);
> +
> + if (!eps) {
> + dev_dbg(_pdev->dev, "found WSMT, but no EPS found\n");
> + return -ENODEV;
> + }
> +
> + /*
> +* Get physical address of buffer and map to virtual address.
> +* Table gives size in 4K pages, regardless of actual system page
> size.
> +*/
> + if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
if (upper_32_bits(..._addr + 8)) {
?
> + dev_warn(_pdev->dev, "found WSMT, but EPS buffer
> address is above 4GB\n");
> + return -EINVAL;
> + }
> + eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,
Why casting?
> +eps->num_of_4k_pages * 4096,
> MEMREMAP_WB);
This multiplication looks strange. Perhaps use PAGE_SIZE?
> + if (!eps_buffer) {
> + dev_warn(_pdev->dev, "found WSMT, but failed to map
> EPS buffer\n");
> + return -ENOMEM;
> + }
> +
> + /* First 8 bytes of buffer is for semaphore */
> + smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;
lower_32_bits() ?
> + smi_data_buf = eps_buffer + 8;
> + smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 -
> 8,
> + (u64) ULONG_MAX);
This is too twisted code. First, it needs explanation.
Second,
Re: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
On Thu, Jun 7, 2018 at 6:59 PM, Stuart Hayes wrote:
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.
> config DCDBAS
> tristate "Dell Systems Management Base Driver"
> - depends on X86
> + depends on X86 && ACPI
Hmm... I'm not sure about this dependency.
So, the question is do all users actually need this? How did it work
previously? How has this been tested in case when command line has
"acpi=off" (yes, this one just for sake of test, I don't believe it's
a real use case)?
> #include
> #include
> #include
> +#include
Please, try to keep an order as much as possible.
For example, in given context this line should be before string.h (I
didn't check the actual file, perhaps even upper).
> #include
>
> #include "dcdbas.h"
> /* Calling Interface SMI */
> - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> + smi_cmd->ebx = smi_data_buf_phys_addr
> + + offsetof(struct smi_cmd, command_buffer);
Please, keep at least + on the previous line.
Also, I'm not sure what is the difference now. Especially for previous
users when this wasn't the part of the driver.
Some explanation needed.
> +static u8 checksum(u8 *buffer, u8 length)
> +{
> + u8 sum = 0;
> + u8 *end = buffer + length;
> +
> + while (buffer < end)
> + sum = (u8)(sum + *(buffer++));
Why not simple
sum += *buffer++;
?
> + return sum;
> +}
And I would rather check if we have similar algoritms already in the
kernel which we might re-use.
> +
> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> +{
> + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> +
> + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
I'm not sure about strings operation here.
I would rather do like with other magic constants: introduce hex value
and compare it as unsigned integer.
Also, it might be a warning, since \0 wasn't ever checked from the
string literal. Though, I'm not sure if it applicable to strncmp()
function (it's for strncpy for sure).
> + return NULL;
> +
> + if (checksum(addr, eps->length) != 0)
> + return NULL;
> +
> + return eps;
> +}
> +
> +static int dcdbas_check_wsmt(void)
> +{
> + struct acpi_table_wsmt *wsmt = NULL;
> + struct smm_eps_table *eps = NULL;
> + u8 *addr;
> +
> + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
> + if (!wsmt)
> + return 0;
> +
> + /* Check if WSMT ACPI table shows that protection is enabled */
> + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> + || !(wsmt->protection_flags
> +& ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION))
> + return 0;
> +
> + /* Scan for EPS (entry point structure) */
> + for (addr = (u8 *)__va(0xf);
> +addr < (u8 *)__va(0x10 - sizeof(struct smm_eps_table)) &&
> !eps;
Perhaps better to do
for (...) {
eps = ...();
if (eps)
break;
}
Also I've a feeling that 0xf constant is defined already somewhere
under arch/x86/include/asm or evem include/linux.
> +addr += 1)
+= 1?!
All tables I saw in BIOS are aligned with 16 bytes. Is it the case here?
Is there any other means to check if the table present? ACPI code?
Method / variable?
> + eps = check_eps_table(addr);
> +
> + if (!eps) {
> + dev_dbg(_pdev->dev, "found WSMT, but no EPS found\n");
> + return -ENODEV;
> + }
> +
> + /*
> +* Get physical address of buffer and map to virtual address.
> +* Table gives size in 4K pages, regardless of actual system page
> size.
> +*/
> + if (eps->smm_comm_buff_addr + 8 > U32_MAX) {
if (upper_32_bits(..._addr + 8)) {
?
> + dev_warn(_pdev->dev, "found WSMT, but EPS buffer
> address is above 4GB\n");
> + return -EINVAL;
> + }
> + eps_buffer = (u8 *)memremap(eps->smm_comm_buff_addr,
Why casting?
> +eps->num_of_4k_pages * 4096,
> MEMREMAP_WB);
This multiplication looks strange. Perhaps use PAGE_SIZE?
> + if (!eps_buffer) {
> + dev_warn(_pdev->dev, "found WSMT, but failed to map
> EPS buffer\n");
> + return -ENOMEM;
> + }
> +
> + /* First 8 bytes of buffer is for semaphore */
> + smi_data_buf_phys_addr = (u32) eps->smm_comm_buff_addr + 8;
lower_32_bits() ?
> + smi_data_buf = eps_buffer + 8;
> + smi_data_buf_size = (unsigned long) min(eps->num_of_4k_pages * 4096 -
> 8,
> + (u64) ULONG_MAX);
This is too twisted code. First, it needs explanation.
Second,
RE: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> -Original Message-
> From: Stuart Hayes [mailto:stuart.w.ha...@gmail.com]
> Sent: Thursday, June 7, 2018 11:00 AM
> To: Warzecha, Douglas
> Cc: Limonciello, Mario; Dominguez, Jared; linux-kernel@vger.kernel.org;
> platform-
> driver-...@vger.kernel.org
> Subject: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
>
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.
>
> Signed-off-by: Stuart Hayes
> Tested-by: Mario Limonciello
> Acked-by: Doug Warzecha
> ---
> v2 Bumped driver version to 5.6.0-3.3
>
>
> drivers/firmware/Kconfig | 2 +-
> drivers/firmware/dcdbas.c | 102
> +++---
> drivers/firmware/dcdbas.h | 11 +
> 3 files changed, 109 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b7c748248e53..a2bd6092bfa1 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -125,7 +125,7 @@ config DELL_RBU
>
> config DCDBAS
> tristate "Dell Systems Management Base Driver"
> - depends on X86
> + depends on X86 && ACPI
> help
> The Dell Systems Management Base Driver provides a sysfs interface
> for systems management software to perform System Management
> diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> index 0bdea60c65dd..1699cfdcefe4 100644
> --- a/drivers/firmware/dcdbas.c
> +++ b/drivers/firmware/dcdbas.c
> @@ -36,12 +36,13 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "dcdbas.h"
>
> #define DRIVER_NAME "dcdbas"
> -#define DRIVER_VERSION "5.6.0-3.2"
> +#define DRIVER_VERSION "5.6.0-3.3"
> #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
>
> static struct platform_device *dcdbas_pdev;
> @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
> static u8 *smi_data_buf;
> static dma_addr_t smi_data_buf_handle;
> static unsigned long smi_data_buf_size;
> +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
> static u32 smi_data_buf_phys_addr;
> static DEFINE_MUTEX(smi_data_lock);
> +static u8 *eps_buffer;
>
> static unsigned int host_control_action;
> static unsigned int host_control_smi_type;
> static unsigned int host_control_on_shutdown;
>
> +static bool wsmt_enabled;
> +
> /**
> * smi_data_buf_free: free SMI data buffer
> */
> static void smi_data_buf_free(void)
> {
> - if (!smi_data_buf)
> + if (!smi_data_buf || wsmt_enabled)
> return;
>
> dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
> @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
> if (smi_data_buf_size >= size)
> return 0;
>
> - if (size > MAX_SMI_DATA_BUF_SIZE)
> + if (size > max_smi_data_buf_size)
> return -EINVAL;
>
> /* new buffer is needed */
> @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct
> kobject
> *kobj,
> {
> ssize_t ret;
>
> - if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> + if ((pos + count) > max_smi_data_buf_size)
> return -EINVAL;
>
> mutex_lock(_data_lock);
> @@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
> break;
> case 1:
> /* Calling Interface SMI */
> - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> + smi_cmd->ebx = smi_data_buf_phys_addr
> + + offsetof(struct smi_cmd, command_buffer);
> ret = dcdbas_smi_request(smi_cmd);
> if (!ret)
> ret = count;
> @@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
> }
> }
>
> +/* WSMT */
> +
> +static u8 checksum(u8 *buffer, u8 length)
> +{
> + u8 sum = 0;
> + u8 *end = buffer + length;
> +
> + while (buffer < end)
> + sum = (u8)(sum + *(buffer++));
> + return sum;
> +}
> +
> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> +{
> + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> +
> + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> + return NULL;
> +
> + if (checksum(addr, eps->length) != 0)
> + return NULL;
> +
> + return eps;
> +}
> +
> +static int dcdbas_check_wsmt(void)
> +{
> + struct acpi_table_wsmt *wsmt = NULL;
> + struct smm_eps_table *eps = NULL;
> + u8 *addr;
> +
> + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
> + if (!wsmt)
> + return 0;
> +
> + /* Check if WSMT ACPI table shows that protection is enabled */
> + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> + || !(wsmt->protection_flags
> + &
RE: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
> -Original Message-
> From: Stuart Hayes [mailto:stuart.w.ha...@gmail.com]
> Sent: Thursday, June 7, 2018 11:00 AM
> To: Warzecha, Douglas
> Cc: Limonciello, Mario; Dominguez, Jared; linux-kernel@vger.kernel.org;
> platform-
> driver-...@vger.kernel.org
> Subject: [PATCH resend v2] dcdbas: Add support for WSMT ACPI table
>
> If the WSMT ACPI table is present and indicates that a fixed communication
> buffer should be used, use the firmware-specified buffer instead of
> allocating a buffer in memory for communications between the dcdbas driver
> and firmare.
>
> Signed-off-by: Stuart Hayes
> Tested-by: Mario Limonciello
> Acked-by: Doug Warzecha
> ---
> v2 Bumped driver version to 5.6.0-3.3
>
>
> drivers/firmware/Kconfig | 2 +-
> drivers/firmware/dcdbas.c | 102
> +++---
> drivers/firmware/dcdbas.h | 11 +
> 3 files changed, 109 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b7c748248e53..a2bd6092bfa1 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -125,7 +125,7 @@ config DELL_RBU
>
> config DCDBAS
> tristate "Dell Systems Management Base Driver"
> - depends on X86
> + depends on X86 && ACPI
> help
> The Dell Systems Management Base Driver provides a sysfs interface
> for systems management software to perform System Management
> diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
> index 0bdea60c65dd..1699cfdcefe4 100644
> --- a/drivers/firmware/dcdbas.c
> +++ b/drivers/firmware/dcdbas.c
> @@ -36,12 +36,13 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "dcdbas.h"
>
> #define DRIVER_NAME "dcdbas"
> -#define DRIVER_VERSION "5.6.0-3.2"
> +#define DRIVER_VERSION "5.6.0-3.3"
> #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver"
>
> static struct platform_device *dcdbas_pdev;
> @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev;
> static u8 *smi_data_buf;
> static dma_addr_t smi_data_buf_handle;
> static unsigned long smi_data_buf_size;
> +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE;
> static u32 smi_data_buf_phys_addr;
> static DEFINE_MUTEX(smi_data_lock);
> +static u8 *eps_buffer;
>
> static unsigned int host_control_action;
> static unsigned int host_control_smi_type;
> static unsigned int host_control_on_shutdown;
>
> +static bool wsmt_enabled;
> +
> /**
> * smi_data_buf_free: free SMI data buffer
> */
> static void smi_data_buf_free(void)
> {
> - if (!smi_data_buf)
> + if (!smi_data_buf || wsmt_enabled)
> return;
>
> dev_dbg(_pdev->dev, "%s: phys: %x size: %lu\n",
> @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size)
> if (smi_data_buf_size >= size)
> return 0;
>
> - if (size > MAX_SMI_DATA_BUF_SIZE)
> + if (size > max_smi_data_buf_size)
> return -EINVAL;
>
> /* new buffer is needed */
> @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct
> kobject
> *kobj,
> {
> ssize_t ret;
>
> - if ((pos + count) > MAX_SMI_DATA_BUF_SIZE)
> + if ((pos + count) > max_smi_data_buf_size)
> return -EINVAL;
>
> mutex_lock(_data_lock);
> @@ -323,7 +328,8 @@ static ssize_t smi_request_store(struct device *dev,
> break;
> case 1:
> /* Calling Interface SMI */
> - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
> + smi_cmd->ebx = smi_data_buf_phys_addr
> + + offsetof(struct smi_cmd, command_buffer);
> ret = dcdbas_smi_request(smi_cmd);
> if (!ret)
> ret = count;
> @@ -482,6 +488,85 @@ static void dcdbas_host_control(void)
> }
> }
>
> +/* WSMT */
> +
> +static u8 checksum(u8 *buffer, u8 length)
> +{
> + u8 sum = 0;
> + u8 *end = buffer + length;
> +
> + while (buffer < end)
> + sum = (u8)(sum + *(buffer++));
> + return sum;
> +}
> +
> +static inline struct smm_eps_table *check_eps_table(u8 *addr)
> +{
> + struct smm_eps_table *eps = (struct smm_eps_table *)addr;
> +
> + if (strncmp(SMM_EPS_SIG, eps->smm_comm_buff_anchor, 4) != 0)
> + return NULL;
> +
> + if (checksum(addr, eps->length) != 0)
> + return NULL;
> +
> + return eps;
> +}
> +
> +static int dcdbas_check_wsmt(void)
> +{
> + struct acpi_table_wsmt *wsmt = NULL;
> + struct smm_eps_table *eps = NULL;
> + u8 *addr;
> +
> + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **));
> + if (!wsmt)
> + return 0;
> +
> + /* Check if WSMT ACPI table shows that protection is enabled */
> + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS)
> + || !(wsmt->protection_flags
> + &
