LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Mingarelli, Thomas" <Thomas.Mingarelli@hp.com>
To: Roland Dreier <rdreier@cisco.com>, Wim Van Sebroeck <wim@iguana.be>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] [WATCHDOG] hpwdt: Use dmi_walk() instead of own copy
Date: Thu, 28 Feb 2008 21:24:48 +0000	[thread overview]
Message-ID: <E14D1C2A44812C4F9C3ED321127EDF650582A5F206@G3W0854.americas.hpqcorp.net> (raw)
In-Reply-To: <ada7igox1jx.fsf_-_@cisco.com>

These changes look good.

Wim, please merge for 2.6.25.

    Acknowledged-by: Thomas Mingarelli <Thomas.Mingarelli@hp.com>


Thanks,

Tom

-----Original Message-----
From: Roland Dreier [mailto:rdreier@cisco.com]
Sent: Thursday, February 28, 2008 2:35 PM
To: Wim Van Sebroeck; Mingarelli, Thomas
Cc: linux-kernel@vger.kernel.org
Subject: [PATCH] [WATCHDOG] hpwdt: Use dmi_walk() instead of own copy

On my HP DL380 G5 system running a 64-bit kernel, loading the hpwdt
driver causes a crash because the driver attempts to ioremap an
invalid physical address.  This is because the driver has an incorrect
definition of the SMBIOS table entry point structure: the table
address is only a 32-bit quantity, and making it a u64 means that the
high-order 32 bits end up containing garbage.

We can fix this by simply deleting all of the duplicated DMI table
walking code and using the kernel's existing dmi_walk() interface to
find the DMI entry the driver is looking for.

Tested on an HP DL380 G5 running a 64-bit kernel.

Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
Wim/Thomas-- Here's an alternative way to fix the crash on 64-bit
systems: just delete all the duplicated DMI walking code, including
the part that has the bug.

 drivers/watchdog/hpwdt.c |  206 +++++-----------------------------------------
 1 files changed, 20 insertions(+), 186 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index a2e174b..2686f3e 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -58,41 +58,6 @@ struct bios32_service_dir {
        u8 reserved[5];
 };

-/*
- * smbios_entry_point     - defines SMBIOS entry point structure
- *
- * anchor[4]              - anchor string (_SM_)
- * checksum               - checksum of the entry point structure
- * length                 - length of the entry point structure
- * major_ver              - major version (02h for revision 2.1)
- * minor_ver              - minor version (01h for revision 2.1)
- * max_struct_size        - size of the largest SMBIOS structure
- * revision               - entry point structure revision implemented
- * formatted_area[5]      - reserved
- * intermediate_anchor[5] - intermediate anchor string (_DMI_)
- * intermediate_checksum  - intermediate checksum
- * table_length           - structure table length
- * table_address          - structure table address
- * table_num_structs      - number of SMBIOS structures present
- * bcd_revision           - BCD revision
- */
-struct smbios_entry_point {
-       u8 anchor[4];
-       u8 checksum;
-       u8 length;
-       u8 major_ver;
-       u8 minor_ver;
-       u16 max_struct_size;
-       u8 revision;
-       u8 formatted_area[5];
-       u8 intermediate_anchor[5];
-       u8 intermediate_checksum;
-       u16 table_length;
-       u64 table_address;
-       u16 table_num_structs;
-       u8 bcd_revision;
-};
-
 /* type 212 */
 struct smbios_cru64_info {
        u8 type;
@@ -175,24 +140,6 @@ static struct pci_device_id hpwdt_devices[] = {
 };
 MODULE_DEVICE_TABLE(pci, hpwdt_devices);

-/*
- *     bios_checksum
- */
-static int __devinit bios_checksum(const char __iomem *ptr, int len)
-{
-       char sum = 0;
-       int i;
-
-       /*
-        * calculate checksum of size bytes. This should add up
-        * to zero if we have a valid header.
-        */
-       for (i = 0; i < len; i++)
-               sum += ptr[i];
-
-       return ((sum == 0) && (len > 0));
-}
-
 #ifndef CONFIG_X86_64
 /* --32 Bit Bios------------------------------------------------------------ */

@@ -303,6 +250,24 @@ static int __devinit cru_detect(unsigned long map_entry,
 }

 /*
+ *     bios_checksum
+ */
+static int __devinit bios_checksum(const char __iomem *ptr, int len)
+{
+       char sum = 0;
+       int i;
+
+       /*
+        * calculate checksum of size bytes. This should add up
+        * to zero if we have a valid header.
+        */
+       for (i = 0; i < len; i++)
+               sum += ptr[i];
+
+       return ((sum == 0) && (len > 0));
+}
+
+/*
  *     bios32_present
  *
  *     Routine Description:
@@ -410,12 +375,8 @@ asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
  *     dmi_find_cru
  *
  *     Routine Description:
- *     This function checks wether or not a SMBIOS/DMI record is
+ *     This function checks whether or not a SMBIOS/DMI record is
  *     the 64bit CRU info or not
- *
- *     Return Value:
- *     0        :  SUCCESS - if record found
- *     <0       :  FAILURE - if record not found
  */
 static void __devinit dmi_find_cru(const struct dmi_header *dm)
 {
@@ -434,138 +395,11 @@ static void __devinit dmi_find_cru(const struct dmi_header *dm)
        }
 }

-/*
- *     dmi_table
- *
- *     Routine Description:
- *     Decode the SMBIOS/DMI table and check if we have a 64bit CRU record
- *     or not.
- *
- *     We have to be cautious here. We have seen BIOSes with DMI pointers
- *     pointing to completely the wrong place for example
- */
-static void __devinit dmi_table(u8 *buf, int len, int num,
-                     void (*decode)(const struct dmi_header *))
-{
-       u8 *data = buf;
-       int i = 0;
-
-       /*
-        *      Stop when we see all the items the table claimed to have
-        *      OR we run off the end of the table (also happens)
-        */
-       while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
-               const struct dmi_header *dm = (const struct dmi_header *)data;
-
-               /*
-                *  We want to know the total length (formated area and strings)
-                *  before decoding to make sure we won't run off the table in
-                *  dmi_decode or dmi_string
-                */
-               data += dm->length;
-               while ((data - buf < len - 1) && (data[0] || data[1]))
-                       data++;
-               if (data - buf < len - 1)
-                       decode(dm);
-               data += 2;
-               i++;
-       }
-}
-
-/*
- *     smbios_present
- *
- *     Routine Description:
- *     This function parses the SMBIOS entry point table to retrieve
- *     the 64 bit CRU Service.
- *
- *     Return Value:
- *     0        :  SUCCESS
- *     <0       :  FAILURE
- */
-static int __devinit smbios_present(const char __iomem *p)
-{
-       struct smbios_entry_point *eps =
-               (struct smbios_entry_point *) p;
-       int length;
-       u8 *buf;
-
-       /* check if we have indeed the SMBIOS table entry point */
-       if ((strncmp((char *)eps->anchor, "_SM_",
-                            sizeof(eps->anchor))) == 0) {
-               length = eps->length;
-
-               /* SMBIOS v2.1 implementation might use 0x1e */
-               if ((length == 0x1e) &&
-                   (eps->major_ver == 2) &&
-                   (eps->minor_ver == 1))
-                       length = 0x1f;
-
-               /*
-                * Now we will check:
-                * - SMBIOS checksum must be 0
-                * - intermediate anchor should be _DMI_
-                * - intermediate checksum should be 0
-                */
-               if ((bios_checksum(p, length)) &&
-                   (strncmp((char *)eps->intermediate_anchor, "_DMI_",
-                            sizeof(eps->intermediate_anchor)) == 0) &&
-                   (bios_checksum(p+0x10, 15))) {
-                       buf = ioremap(eps->table_address, eps->table_length);
-                       if (buf == NULL)
-                               return -ENODEV;
-
-
-                       /* Scan the DMI table for the 64 bit CRU service */
-                       dmi_table(buf, eps->table_length,
-                                 eps->table_num_structs, dmi_find_cru);
-
-                       iounmap(buf);
-                       return 0;
-               }
-       }
-
-       return -ENODEV;
-}
-
-static int __devinit smbios_scan_machine(void)
-{
-       char __iomem *p, *q;
-       int rc;
-
-       if (efi_enabled) {
-               if (efi.smbios == EFI_INVALID_TABLE_ADDR)
-                       return -ENODEV;
-
-               p = ioremap(efi.smbios, 32);
-               if (p == NULL)
-                       return -ENOMEM;
-
-               rc = smbios_present(p);
-               iounmap(p);
-       } else {
-               /*
-                * Search from 0x0f0000 through 0x0fffff, inclusive.
-                */
-               p = ioremap(PCI_ROM_BASE1, ROM_SIZE);
-               if (p == NULL)
-                       return -ENOMEM;
-
-               for (q = p; q < p + ROM_SIZE; q += 16) {
-                       rc = smbios_present(q);
-                       if (!rc) {
-                               break;
-                       }
-               }
-               iounmap(p);
-       }
-}
-
 static int __devinit detect_cru_service(void)
 {
        cru_rom_addr = NULL;

-       smbios_scan_machine();  /* will become dmi_walk(dmi_find_cru); */
+       dmi_walk(dmi_find_cru);

        /* if cru_rom_addr has been set then we found a CRU service */
        return ((cru_rom_addr != NULL)? 0: -ENODEV);

  reply	other threads:[~2008-02-28 21:26 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-27 17:08 hpwdt oops in clflush_cache_range Roland Dreier
2008-02-27 17:37 ` Mingarelli, Thomas
2008-02-27 18:14 ` Thomas Gleixner
2008-02-27 18:38   ` Roland Dreier
2008-02-27 19:48     ` Thomas Gleixner
2008-02-27 20:36       ` Ingo Molnar
2008-02-27 20:42         ` Thomas Gleixner
2008-02-27 20:44           ` Ingo Molnar
2008-02-27 20:59           ` Thomas Gleixner
2008-02-27 21:14             ` Ingo Molnar
2008-02-27 21:17             ` Roland Dreier
2008-02-27 21:35               ` Roland Dreier
2008-02-27 23:44               ` Mingarelli, Thomas
2008-02-28  0:12                 ` Roland Dreier
2008-02-28  3:09                   ` Mingarelli, Thomas
     [not found]                   ` <E14D1C2A44812C4F9C3ED321127EDF6505829D5D6C@G3W0854.americas.hpqcorp.net>
2008-02-28 17:38                     ` [PATCH] [WATCHDOG] Fix declaration of struct smbios_entry_point in hpwdt Roland Dreier
2008-02-28 17:48                       ` [PATCH for 2.6.26] [WATCHDOG] Fix return value warning " Roland Dreier
2008-02-28 20:34                       ` [PATCH] [WATCHDOG] hpwdt: Use dmi_walk() instead of own copy Roland Dreier
2008-02-28 21:24                         ` Mingarelli, Thomas [this message]
2008-02-28 21:26                         ` Wim Van Sebroeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E14D1C2A44812C4F9C3ED321127EDF650582A5F206@G3W0854.americas.hpqcorp.net \
    --to=thomas.mingarelli@hp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rdreier@cisco.com \
    --cc=wim@iguana.be \
    --subject='RE: [PATCH] [WATCHDOG] hpwdt: Use dmi_walk() instead of own copy' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).