* [PATCH 0/4] ipmi: Allow dynamic device IDs
@ 2016-09-12 8:55 Jeremy Kerr
2016-09-12 8:55 ` [PATCH 1/4] ipmi: Add a reference from BMC devices to their interfaces Jeremy Kerr
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Jeremy Kerr @ 2016-09-12 8:55 UTC (permalink / raw)
To: Corey Minyard, openipmi-developer; +Cc: linuxppc-dev
This series implements a more dynamic method of reporting BMC version &
identification. The current method of registering versions and IDs
during ipmi_register_smi means that if a BMC is upgraded during a boot,
the IPMI core code will report old version information.
We do this by querying the BMC (using a Get Device ID request) at smi
registration in the IPMI core code, and when the sysfs version & id
attributes are accessed.
The core of the change is in patch 3/4. Patches 1 and 2 implement a
couple of minor API changes leading up to this.
Patch 4 converts the powernv IPMI driver to use the dynamic IDs; the
behaviour of the other SMIs is not changed by this series. However,
if there's interest, I'm happy to alter the existing SMIs too, in a
follow-up series.
Questions & comments most welcome.
Cheers,
Jeremy
---
Jeremy Kerr (4):
ipmi: Add a reference from BMC devices to their interfaces
ipmi: Make ipmi_demangle_device_id more generic
ipmi: allow dynamic BMC version information
ipmi/powernv: Use dynamic device ids
drivers/char/ipmi/ipmi_msghandler.c | 160 ++++++++++++++++++++++++++++++++++--
drivers/char/ipmi/ipmi_powernv.c | 5 +-
drivers/char/ipmi/ipmi_si_intf.c | 3 +-
drivers/char/ipmi/ipmi_ssif.c | 3 +-
include/linux/ipmi_smi.h | 16 ++--
5 files changed, 169 insertions(+), 18 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/4] ipmi: Add a reference from BMC devices to their interfaces 2016-09-12 8:55 [PATCH 0/4] ipmi: Allow dynamic device IDs Jeremy Kerr @ 2016-09-12 8:55 ` Jeremy Kerr 2016-09-12 8:55 ` [PATCH 2/4] ipmi: Make ipmi_demangle_device_id more generic Jeremy Kerr ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ messages in thread From: Jeremy Kerr @ 2016-09-12 8:55 UTC (permalink / raw) To: Corey Minyard, openipmi-developer; +Cc: linuxppc-dev In an upcoming change, we'll want to grab a reference to the ipmi_smi_t from a struct bmc_device. This change adds a pointer to allow this. Signed-off-by: Jeremy Kerr <jk@ozlabs.org> --- drivers/char/ipmi/ipmi_msghandler.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index d8619998..41990bb 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -195,6 +195,7 @@ struct ipmi_proc_entry { struct bmc_device { struct platform_device pdev; struct ipmi_device_id id; + ipmi_smi_t intf; unsigned char guid[16]; int guid_set; char name[16]; @@ -2796,6 +2797,7 @@ int ipmi_register_smi(const struct ipmi_smi_handlers *handlers, } intf->intf_num = -1; /* Mark it invalid for now. */ kref_init(&intf->refcount); + intf->bmc->intf = intf; intf->bmc->id = *device_id; intf->si_dev = si_dev; for (j = 0; j < IPMI_MAX_CHANNELS; j++) { -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/4] ipmi: Make ipmi_demangle_device_id more generic 2016-09-12 8:55 [PATCH 0/4] ipmi: Allow dynamic device IDs Jeremy Kerr 2016-09-12 8:55 ` [PATCH 1/4] ipmi: Add a reference from BMC devices to their interfaces Jeremy Kerr @ 2016-09-12 8:55 ` Jeremy Kerr 2016-09-12 8:55 ` [PATCH 3/4] ipmi: allow dynamic BMC version information Jeremy Kerr 2016-09-12 8:55 ` [PATCH 4/4] ipmi/powernv: Use dynamic device ids Jeremy Kerr 3 siblings, 0 replies; 7+ messages in thread From: Jeremy Kerr @ 2016-09-12 8:55 UTC (permalink / raw) To: Corey Minyard, openipmi-developer; +Cc: linuxppc-dev Currently, ipmi_demagle_device_id requires a full response buffer in its data argument. This means we can't use it to parse a response in a struct ipmi_recv_msg, which has the netfn and cmd as separate bytes. This change alters the definition and users of ipmi_demangle_device_id to use a split netfn, cmd and data buffer, so it can be used with non-sequential responses. Signed-off-by: Jeremy Kerr <jk@ozlabs.org> --- drivers/char/ipmi/ipmi_si_intf.c | 3 ++- drivers/char/ipmi/ipmi_ssif.c | 3 ++- include/linux/ipmi_smi.h | 16 +++++++++------- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index a112c01..745368d 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -2951,7 +2951,8 @@ static int try_get_dev_id(struct smi_info *smi_info) resp, IPMI_MAX_MSG_LENGTH); /* Check and record info from the get device id, in case we need it. */ - rv = ipmi_demangle_device_id(resp, resp_len, &smi_info->device_id); + rv = ipmi_demangle_device_id(msg[0], msg[1], + resp+2, resp_len-2, &smi_info->device_id); out: kfree(resp); diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 5673fff..29bbcaf 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -1461,7 +1461,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) if (rv) goto out; - rv = ipmi_demangle_device_id(resp, len, &ssif_info->device_id); + rv = ipmi_demangle_device_id(msg[0], msg[1], + resp+2, len-2, &ssif_info->device_id); if (rv) goto out; diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h index f8cea14..bf2f489 100644 --- a/include/linux/ipmi_smi.h +++ b/include/linux/ipmi_smi.h @@ -167,22 +167,24 @@ struct ipmi_device_id { netfn << 2, the data should be of the format: netfn << 2, cmd, completion code, data as normally comes from a device interface. */ -static inline int ipmi_demangle_device_id(const unsigned char *data, +static inline int ipmi_demangle_device_id(uint8_t netfn, uint8_t cmd, + const unsigned char *data, unsigned int data_len, struct ipmi_device_id *id) { - if (data_len < 9) + if (data_len < 7) return -EINVAL; - if (data[0] != IPMI_NETFN_APP_RESPONSE << 2 || - data[1] != IPMI_GET_DEVICE_ID_CMD) + if (netfn != IPMI_NETFN_APP_RESPONSE << 2 || + cmd != IPMI_GET_DEVICE_ID_CMD) /* Strange, didn't get the response we expected. */ return -EINVAL; - if (data[2] != 0) + if (data[0] != 0) /* That's odd, it shouldn't be able to fail. */ return -EINVAL; - data += 3; - data_len -= 3; + data++; + data_len--; + id->device_id = data[0]; id->device_revision = data[1]; id->firmware_revision_1 = data[2]; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/4] ipmi: allow dynamic BMC version information 2016-09-12 8:55 [PATCH 0/4] ipmi: Allow dynamic device IDs Jeremy Kerr 2016-09-12 8:55 ` [PATCH 1/4] ipmi: Add a reference from BMC devices to their interfaces Jeremy Kerr 2016-09-12 8:55 ` [PATCH 2/4] ipmi: Make ipmi_demangle_device_id more generic Jeremy Kerr @ 2016-09-12 8:55 ` Jeremy Kerr 2016-09-12 16:38 ` [Openipmi-developer] " Corey Minyard 2016-09-12 8:55 ` [PATCH 4/4] ipmi/powernv: Use dynamic device ids Jeremy Kerr 3 siblings, 1 reply; 7+ messages in thread From: Jeremy Kerr @ 2016-09-12 8:55 UTC (permalink / raw) To: Corey Minyard, openipmi-developer; +Cc: linuxppc-dev Currently, it's up to the IPMI SMIs to provide the product & version details of BMCs behind registered IPMI SMI interfaces. This device ID is provided on SMI regsitration, and kept around for all future queries. However, this version information isn't always static. For example, a BMC may be upgraded at runtime, making the old version information stale. This change allows querying the BMC device ID & version information dynamically. If no static device_id argument is provided to ipmi_register_smi, then the IPMI core code will perform a Get Device ID IPMI command to query the version information when needed. We keep a short-term cache of this information so we don't need to re-query for every attribute access. Signed-off-by: Jeremy Kerr <jk@ozlabs.org> --- drivers/char/ipmi/ipmi_msghandler.c | 158 ++++++++++++++++++++++++++++++++++-- 1 file changed, 153 insertions(+), 5 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 41990bb..55feba0 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -90,6 +90,9 @@ static struct proc_dir_entry *proc_ipmi_root; */ #define IPMI_REQUEST_EV_TIME (1000 / (IPMI_TIMEOUT_TIME)) +/* How long should we cache dynamic device IDs? */ +#define IPMI_DYN_DEV_ID_EXPIRY (10 * HZ) + /* * The main "user" data structure. */ @@ -192,10 +195,19 @@ struct ipmi_proc_entry { }; #endif +enum bmc_dyn_device_id_state { + BMC_DEVICE_DYN_ID_STATE_DISABLED, + BMC_DEVICE_DYN_ID_STATE_QUERYING, + BMC_DEVICE_DYN_ID_STATE_VALID, + BMC_DEVICE_DYN_ID_STATE_INVALID, +}; + struct bmc_device { struct platform_device pdev; struct ipmi_device_id id; ipmi_smi_t intf; + int dyn_id; + unsigned long dyn_id_expiry; unsigned char guid[16]; int guid_set; char name[16]; @@ -1997,6 +2009,108 @@ int ipmi_request_supply_msgs(ipmi_user_t user, } EXPORT_SYMBOL(ipmi_request_supply_msgs); +static void bmc_device_id_handler(ipmi_smi_t intf, struct ipmi_recv_msg *msg) +{ + int rc; + + if ((msg->addr.addr_type != IPMI_SYSTEM_INTERFACE_ADDR_TYPE) + || (msg->msg.netfn != IPMI_NETFN_APP_RESPONSE) + || (msg->msg.cmd != IPMI_GET_DEVICE_ID_CMD)) + return; + + /* Do we have a success completion code? */ + if (msg->msg.data[0] != 0) { + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; + goto out; + } + + /* Do we have enough data to parse the device ID details? This doesn't + * inclde the optional auxilliary version data. */ + if (msg->msg.data_len < 12) { + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; + goto out; + } + + rc = ipmi_demangle_device_id(msg->msg.netfn, msg->msg.cmd, + msg->msg.data, msg->msg.data_len, &intf->bmc->id); + if (rc) { + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; + goto out; + } + + intf->ipmi_version_major = ipmi_version_major(&intf->bmc->id); + intf->ipmi_version_minor = ipmi_version_minor(&intf->bmc->id); + + /* All good! mark the dynamic ID as valid, and set its expiration + * time */ + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_VALID; + intf->bmc->dyn_id_expiry = jiffies + IPMI_DYN_DEV_ID_EXPIRY; +out: + wake_up(&intf->waitq); +} + + +static int bmc_update_device_id(struct bmc_device *bmc) +{ + struct ipmi_system_interface_addr si; + ipmi_smi_t intf = bmc->intf; + struct kernel_ipmi_msg msg; + int rc; + + if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_DISABLED) + return 0; + + if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_VALID && + time_is_after_jiffies(bmc->dyn_id_expiry)) + return 0; + + /* do we have a request in progress? Just wait for that. */ + if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_QUERYING) + return wait_event_timeout(intf->waitq, + bmc->dyn_id != BMC_DEVICE_DYN_ID_STATE_QUERYING, + IPMI_DYN_DEV_ID_EXPIRY); + + /* send Get Device ID request */ + si.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE; + si.channel = IPMI_BMC_CHANNEL; + si.lun = 0; + + msg.netfn = IPMI_NETFN_APP_REQUEST; + msg.cmd = IPMI_GET_DEVICE_ID_CMD; + msg.data = NULL; + msg.data_len = 0; + + intf->null_user_handler = bmc_device_id_handler; + + bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_QUERYING; + + rc = i_ipmi_request(NULL, + intf, + (struct ipmi_addr *) &si, + 0, + &msg, + intf, + NULL, + NULL, + 0, + intf->channels[0].address, + intf->channels[0].lun, + -1, 0); + + if (rc) { + bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; + return rc; + } + + wait_event_timeout(intf->waitq, + bmc->dyn_id != BMC_DEVICE_DYN_ID_STATE_QUERYING, + IPMI_DYN_DEV_ID_EXPIRY); + + intf->null_user_handler = NULL; + + return bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_VALID ? 0 : 1; +} + #ifdef CONFIG_PROC_FS static int smi_ipmb_proc_show(struct seq_file *m, void *v) { @@ -2273,6 +2387,9 @@ static ssize_t provides_device_sdrs_show(struct device *dev, { struct bmc_device *bmc = to_bmc_device(dev); + if (bmc_update_device_id(bmc)) + return -EIO; + return snprintf(buf, 10, "%u\n", (bmc->id.device_revision & 0x80) >> 7); } @@ -2284,6 +2401,9 @@ static ssize_t revision_show(struct device *dev, struct device_attribute *attr, { struct bmc_device *bmc = to_bmc_device(dev); + if (bmc_update_device_id(bmc)) + return -EIO; + return snprintf(buf, 20, "%u\n", bmc->id.device_revision & 0x0F); } @@ -2295,6 +2415,9 @@ static ssize_t firmware_revision_show(struct device *dev, { struct bmc_device *bmc = to_bmc_device(dev); + if (bmc_update_device_id(bmc)) + return -EIO; + return snprintf(buf, 20, "%u.%x\n", bmc->id.firmware_revision_1, bmc->id.firmware_revision_2); } @@ -2306,6 +2429,9 @@ static ssize_t ipmi_version_show(struct device *dev, { struct bmc_device *bmc = to_bmc_device(dev); + if (bmc_update_device_id(bmc)) + return -EIO; + return snprintf(buf, 20, "%u.%u\n", ipmi_version_major(&bmc->id), ipmi_version_minor(&bmc->id)); @@ -2318,6 +2444,9 @@ static ssize_t add_dev_support_show(struct device *dev, { struct bmc_device *bmc = to_bmc_device(dev); + if (bmc_update_device_id(bmc)) + return -EIO; + return snprintf(buf, 10, "0x%02x\n", bmc->id.additional_device_support); } @@ -2330,6 +2459,9 @@ static ssize_t manufacturer_id_show(struct device *dev, { struct bmc_device *bmc = to_bmc_device(dev); + if (bmc_update_device_id(bmc)) + return -EIO; + return snprintf(buf, 20, "0x%6.6x\n", bmc->id.manufacturer_id); } static DEVICE_ATTR(manufacturer_id, S_IRUGO, manufacturer_id_show, NULL); @@ -2340,6 +2472,9 @@ static ssize_t product_id_show(struct device *dev, { struct bmc_device *bmc = to_bmc_device(dev); + if (bmc_update_device_id(bmc)) + return -EIO; + return snprintf(buf, 10, "0x%4.4x\n", bmc->id.product_id); } static DEVICE_ATTR(product_id, S_IRUGO, product_id_show, NULL); @@ -2350,6 +2485,9 @@ static ssize_t aux_firmware_rev_show(struct device *dev, { struct bmc_device *bmc = to_bmc_device(dev); + if (bmc_update_device_id(bmc)) + return -EIO; + return snprintf(buf, 21, "0x%02x 0x%02x 0x%02x 0x%02x\n", bmc->id.aux_firmware_revision[3], bmc->id.aux_firmware_revision[2], @@ -2390,8 +2528,10 @@ static umode_t bmc_dev_attr_is_visible(struct kobject *kobj, struct bmc_device *bmc = to_bmc_device(dev); umode_t mode = attr->mode; - if (attr == &dev_attr_aux_firmware_revision.attr) + if (attr == &dev_attr_aux_firmware_revision.attr) { + bmc_update_device_id(bmc); return bmc->id.aux_firmware_revision_set ? mode : 0; + } if (attr == &dev_attr_guid.attr) return bmc->guid_set ? mode : 0; return mode; @@ -2450,6 +2590,8 @@ static int ipmi_bmc_register(ipmi_smi_t intf, int ifnum) mutex_lock(&ipmidriver_mutex); + bmc_update_device_id(bmc); + /* * Try to find if there is an bmc_device struct * representing the interfaced BMC already @@ -2787,9 +2929,6 @@ int ipmi_register_smi(const struct ipmi_smi_handlers *handlers, if (!intf) return -ENOMEM; - intf->ipmi_version_major = ipmi_version_major(device_id); - intf->ipmi_version_minor = ipmi_version_minor(device_id); - intf->bmc = kzalloc(sizeof(*intf->bmc), GFP_KERNEL); if (!intf->bmc) { kfree(intf); @@ -2798,7 +2937,16 @@ int ipmi_register_smi(const struct ipmi_smi_handlers *handlers, intf->intf_num = -1; /* Mark it invalid for now. */ kref_init(&intf->refcount); intf->bmc->intf = intf; - intf->bmc->id = *device_id; + if (device_id) { + intf->bmc->id = *device_id; + intf->ipmi_version_major = ipmi_version_major(device_id); + intf->ipmi_version_minor = ipmi_version_minor(device_id); + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_DISABLED; + } else { + memset(&intf->bmc->id, 0, sizeof(intf->bmc->id)); + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; + } + intf->si_dev = si_dev; for (j = 0; j < IPMI_MAX_CHANNELS; j++) { intf->channels[j].address = IPMI_BMC_SLAVE_ADDR; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Openipmi-developer] [PATCH 3/4] ipmi: allow dynamic BMC version information 2016-09-12 8:55 ` [PATCH 3/4] ipmi: allow dynamic BMC version information Jeremy Kerr @ 2016-09-12 16:38 ` Corey Minyard 2016-09-13 10:14 ` Jeremy Kerr 0 siblings, 1 reply; 7+ messages in thread From: Corey Minyard @ 2016-09-12 16:38 UTC (permalink / raw) To: Jeremy Kerr, openipmi-developer; +Cc: linuxppc-dev On 09/12/2016 03:55 AM, Jeremy Kerr wrote: > Currently, it's up to the IPMI SMIs to provide the product & version > details of BMCs behind registered IPMI SMI interfaces. This device ID is > provided on SMI regsitration, and kept around for all future queries. > > However, this version information isn't always static. For example, a > BMC may be upgraded at runtime, making the old version information > stale. > > This change allows querying the BMC device ID & version information > dynamically. If no static device_id argument is provided to > ipmi_register_smi, then the IPMI core code will perform a Get Device ID > IPMI command to query the version information when needed. We keep a > short-term cache of this information so we don't need to re-query > for every attribute access. In all, this looks good. I have two minor nits inline below, and two more major comments here: I would prefer if it always queried the data, even if the device id information is provided by the lower level driver. The other two interfaces query the device id for blacklisting and interface detection, so they already have it available. If the dev_id is provided, you can just set the information valid and set the last query time. Then the disabled state is no longer necessary. Since the values can change while you are querying them, do we need some sort of mutex on them? I know the chances are pretty remote that you would ever hit an issue, but it's at least theoretically possible. Thanks, -corey > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > --- > drivers/char/ipmi/ipmi_msghandler.c | 158 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 153 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c > index 41990bb..55feba0 100644 > --- a/drivers/char/ipmi/ipmi_msghandler.c > +++ b/drivers/char/ipmi/ipmi_msghandler.c > @@ -90,6 +90,9 @@ static struct proc_dir_entry *proc_ipmi_root; > */ > #define IPMI_REQUEST_EV_TIME (1000 / (IPMI_TIMEOUT_TIME)) > > +/* How long should we cache dynamic device IDs? */ > +#define IPMI_DYN_DEV_ID_EXPIRY (10 * HZ) > + > /* > * The main "user" data structure. > */ > @@ -192,10 +195,19 @@ struct ipmi_proc_entry { > }; > #endif > > +enum bmc_dyn_device_id_state { > + BMC_DEVICE_DYN_ID_STATE_DISABLED, > + BMC_DEVICE_DYN_ID_STATE_QUERYING, > + BMC_DEVICE_DYN_ID_STATE_VALID, > + BMC_DEVICE_DYN_ID_STATE_INVALID, > +}; > + > struct bmc_device { > struct platform_device pdev; > struct ipmi_device_id id; > ipmi_smi_t intf; > + int dyn_id; > + unsigned long dyn_id_expiry; > unsigned char guid[16]; > int guid_set; > char name[16]; > @@ -1997,6 +2009,108 @@ int ipmi_request_supply_msgs(ipmi_user_t user, > } > EXPORT_SYMBOL(ipmi_request_supply_msgs); > > +static void bmc_device_id_handler(ipmi_smi_t intf, struct ipmi_recv_msg *msg) > +{ > + int rc; > + > + if ((msg->addr.addr_type != IPMI_SYSTEM_INTERFACE_ADDR_TYPE) > + || (msg->msg.netfn != IPMI_NETFN_APP_RESPONSE) > + || (msg->msg.cmd != IPMI_GET_DEVICE_ID_CMD)) > + return; > + > + /* Do we have a success completion code? */ > + if (msg->msg.data[0] != 0) { > + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; > + goto out; > + } > + > + /* Do we have enough data to parse the device ID details? This doesn't > + * inclde the optional auxilliary version data. */ Minor nit: include is misspelled. > + if (msg->msg.data_len < 12) { > + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; > + goto out; > + } > + > + rc = ipmi_demangle_device_id(msg->msg.netfn, msg->msg.cmd, > + msg->msg.data, msg->msg.data_len, &intf->bmc->id); > + if (rc) { > + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; > + goto out; > + } > + > + intf->ipmi_version_major = ipmi_version_major(&intf->bmc->id); > + intf->ipmi_version_minor = ipmi_version_minor(&intf->bmc->id); > + > + /* All good! mark the dynamic ID as valid, and set its expiration > + * time */ > + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_VALID; > + intf->bmc->dyn_id_expiry = jiffies + IPMI_DYN_DEV_ID_EXPIRY; > +out: > + wake_up(&intf->waitq); > +} > + > + > +static int bmc_update_device_id(struct bmc_device *bmc) > +{ > + struct ipmi_system_interface_addr si; > + ipmi_smi_t intf = bmc->intf; > + struct kernel_ipmi_msg msg; > + int rc; > + > + if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_DISABLED) > + return 0; > + > + if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_VALID && > + time_is_after_jiffies(bmc->dyn_id_expiry)) > + return 0; > + > + /* do we have a request in progress? Just wait for that. */ > + if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_QUERYING) > + return wait_event_timeout(intf->waitq, > + bmc->dyn_id != BMC_DEVICE_DYN_ID_STATE_QUERYING, > + IPMI_DYN_DEV_ID_EXPIRY); > + > + /* send Get Device ID request */ > + si.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE; > + si.channel = IPMI_BMC_CHANNEL; > + si.lun = 0; > + > + msg.netfn = IPMI_NETFN_APP_REQUEST; > + msg.cmd = IPMI_GET_DEVICE_ID_CMD; > + msg.data = NULL; > + msg.data_len = 0; > + > + intf->null_user_handler = bmc_device_id_handler; > + > + bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_QUERYING; > + > + rc = i_ipmi_request(NULL, > + intf, > + (struct ipmi_addr *) &si, > + 0, > + &msg, > + intf, > + NULL, > + NULL, > + 0, > + intf->channels[0].address, > + intf->channels[0].lun, > + -1, 0); > + > + if (rc) { > + bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; > + return rc; > + } > + > + wait_event_timeout(intf->waitq, > + bmc->dyn_id != BMC_DEVICE_DYN_ID_STATE_QUERYING, > + IPMI_DYN_DEV_ID_EXPIRY); > + > + intf->null_user_handler = NULL; > + > + return bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_VALID ? 0 : 1; > +} > + > #ifdef CONFIG_PROC_FS > static int smi_ipmb_proc_show(struct seq_file *m, void *v) > { > @@ -2273,6 +2387,9 @@ static ssize_t provides_device_sdrs_show(struct device *dev, > { > struct bmc_device *bmc = to_bmc_device(dev); > > + if (bmc_update_device_id(bmc)) > + return -EIO; > + > return snprintf(buf, 10, "%u\n", > (bmc->id.device_revision & 0x80) >> 7); > } > @@ -2284,6 +2401,9 @@ static ssize_t revision_show(struct device *dev, struct device_attribute *attr, > { > struct bmc_device *bmc = to_bmc_device(dev); > > + if (bmc_update_device_id(bmc)) > + return -EIO; > + > return snprintf(buf, 20, "%u\n", > bmc->id.device_revision & 0x0F); > } > @@ -2295,6 +2415,9 @@ static ssize_t firmware_revision_show(struct device *dev, > { > struct bmc_device *bmc = to_bmc_device(dev); > > + if (bmc_update_device_id(bmc)) > + return -EIO; > + > return snprintf(buf, 20, "%u.%x\n", bmc->id.firmware_revision_1, > bmc->id.firmware_revision_2); > } > @@ -2306,6 +2429,9 @@ static ssize_t ipmi_version_show(struct device *dev, > { > struct bmc_device *bmc = to_bmc_device(dev); > > + if (bmc_update_device_id(bmc)) > + return -EIO; > + > return snprintf(buf, 20, "%u.%u\n", > ipmi_version_major(&bmc->id), > ipmi_version_minor(&bmc->id)); > @@ -2318,6 +2444,9 @@ static ssize_t add_dev_support_show(struct device *dev, > { > struct bmc_device *bmc = to_bmc_device(dev); > > + if (bmc_update_device_id(bmc)) > + return -EIO; > + > return snprintf(buf, 10, "0x%02x\n", > bmc->id.additional_device_support); > } > @@ -2330,6 +2459,9 @@ static ssize_t manufacturer_id_show(struct device *dev, > { > struct bmc_device *bmc = to_bmc_device(dev); > > + if (bmc_update_device_id(bmc)) > + return -EIO; > + > return snprintf(buf, 20, "0x%6.6x\n", bmc->id.manufacturer_id); > } > static DEVICE_ATTR(manufacturer_id, S_IRUGO, manufacturer_id_show, NULL); > @@ -2340,6 +2472,9 @@ static ssize_t product_id_show(struct device *dev, > { > struct bmc_device *bmc = to_bmc_device(dev); > > + if (bmc_update_device_id(bmc)) > + return -EIO; > + > return snprintf(buf, 10, "0x%4.4x\n", bmc->id.product_id); > } > static DEVICE_ATTR(product_id, S_IRUGO, product_id_show, NULL); > @@ -2350,6 +2485,9 @@ static ssize_t aux_firmware_rev_show(struct device *dev, > { > struct bmc_device *bmc = to_bmc_device(dev); > > + if (bmc_update_device_id(bmc)) > + return -EIO; > + > return snprintf(buf, 21, "0x%02x 0x%02x 0x%02x 0x%02x\n", > bmc->id.aux_firmware_revision[3], > bmc->id.aux_firmware_revision[2], > @@ -2390,8 +2528,10 @@ static umode_t bmc_dev_attr_is_visible(struct kobject *kobj, > struct bmc_device *bmc = to_bmc_device(dev); > umode_t mode = attr->mode; > > - if (attr == &dev_attr_aux_firmware_revision.attr) > + if (attr == &dev_attr_aux_firmware_revision.attr) { > + bmc_update_device_id(bmc); > return bmc->id.aux_firmware_revision_set ? mode : 0; > + } > if (attr == &dev_attr_guid.attr) > return bmc->guid_set ? mode : 0; > return mode; > @@ -2450,6 +2590,8 @@ static int ipmi_bmc_register(ipmi_smi_t intf, int ifnum) > > mutex_lock(&ipmidriver_mutex); > > + bmc_update_device_id(bmc); > + What happens if this fails? You can return an error from this function. It's also probably not necessary to have this inside the mutex. > /* > * Try to find if there is an bmc_device struct > * representing the interfaced BMC already > @@ -2787,9 +2929,6 @@ int ipmi_register_smi(const struct ipmi_smi_handlers *handlers, > if (!intf) > return -ENOMEM; > > - intf->ipmi_version_major = ipmi_version_major(device_id); > - intf->ipmi_version_minor = ipmi_version_minor(device_id); > - > intf->bmc = kzalloc(sizeof(*intf->bmc), GFP_KERNEL); > if (!intf->bmc) { > kfree(intf); > @@ -2798,7 +2937,16 @@ int ipmi_register_smi(const struct ipmi_smi_handlers *handlers, > intf->intf_num = -1; /* Mark it invalid for now. */ > kref_init(&intf->refcount); > intf->bmc->intf = intf; > - intf->bmc->id = *device_id; > + if (device_id) { > + intf->bmc->id = *device_id; > + intf->ipmi_version_major = ipmi_version_major(device_id); > + intf->ipmi_version_minor = ipmi_version_minor(device_id); > + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_DISABLED; > + } else { > + memset(&intf->bmc->id, 0, sizeof(intf->bmc->id)); > + intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID; > + } > + > intf->si_dev = si_dev; > for (j = 0; j < IPMI_MAX_CHANNELS; j++) { > intf->channels[j].address = IPMI_BMC_SLAVE_ADDR; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Openipmi-developer] [PATCH 3/4] ipmi: allow dynamic BMC version information 2016-09-12 16:38 ` [Openipmi-developer] " Corey Minyard @ 2016-09-13 10:14 ` Jeremy Kerr 0 siblings, 0 replies; 7+ messages in thread From: Jeremy Kerr @ 2016-09-13 10:14 UTC (permalink / raw) To: minyard, openipmi-developer; +Cc: linuxppc-dev Hi Corey, > In all, this looks good. I have two minor nits inline below, and > two more major comments here: > > I would prefer if it always queried the data, even if the device id > information is provided by the lower level driver. OK, can do. I was concerned that the SMI driver may want to provide its own device identification details (and not want to override those with the ones provided by Get Device Id), but if that's not the case then it makes sense to store the original values but allow requery. > Since the values can change while you are querying them, do > we need some sort of mutex on them? Which values are you referring to here? The device ID, or the members of struct bmc_device? If it's the latter, I'd be happy to take your guidance on the locking protocol there. Is there an existing mutex that would be suitable for that? >From the comments inline: > > + /* Do we have enough data to parse the device ID details? This doesn't > > + * inclde the optional auxilliary version data. */ > > Minor nit: include is misspelled. D'oh, will fix in v2. > > @@ -2450,6 +2590,8 @@ static int ipmi_bmc_register(ipmi_smi_t intf, int ifnum) > > > > mutex_lock(&ipmidriver_mutex); > > > > + bmc_update_device_id(bmc); > > + > > What happens if this fails? You can return an error from this function. > It's also probably not necessary to have this inside the mutex. I didn't think we'd want to fail registration on query failure there; it could be a transient error. Cheers, Jeremy ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4/4] ipmi/powernv: Use dynamic device ids 2016-09-12 8:55 [PATCH 0/4] ipmi: Allow dynamic device IDs Jeremy Kerr ` (2 preceding siblings ...) 2016-09-12 8:55 ` [PATCH 3/4] ipmi: allow dynamic BMC version information Jeremy Kerr @ 2016-09-12 8:55 ` Jeremy Kerr 3 siblings, 0 replies; 7+ messages in thread From: Jeremy Kerr @ 2016-09-12 8:55 UTC (permalink / raw) To: Corey Minyard, openipmi-developer; +Cc: linuxppc-dev We don't currently provide any device ID info in the SMI registration, so use the dynamic ID infrastructure instead. Signed-off-by: Jeremy Kerr <jk@ozlabs.org> --- drivers/char/ipmi/ipmi_powernv.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/char/ipmi/ipmi_powernv.c b/drivers/char/ipmi/ipmi_powernv.c index 6e658aa..10e69d6 100644 --- a/drivers/char/ipmi/ipmi_powernv.c +++ b/drivers/char/ipmi/ipmi_powernv.c @@ -23,7 +23,6 @@ struct ipmi_smi_powernv { u64 interface_id; - struct ipmi_device_id ipmi_id; ipmi_smi_t intf; unsigned int irq; @@ -265,9 +264,7 @@ static int ipmi_powernv_probe(struct platform_device *pdev) goto err_unregister; } - /* todo: query actual ipmi_device_id */ - rc = ipmi_register_smi(&ipmi_powernv_smi_handlers, ipmi, - &ipmi->ipmi_id, dev, 0); + rc = ipmi_register_smi(&ipmi_powernv_smi_handlers, ipmi, NULL, dev, 0); if (rc) { dev_warn(dev, "IPMI SMI registration failed (%d)\n", rc); goto err_free_msg; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-13 10:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-12 8:55 [PATCH 0/4] ipmi: Allow dynamic device IDs Jeremy Kerr 2016-09-12 8:55 ` [PATCH 1/4] ipmi: Add a reference from BMC devices to their interfaces Jeremy Kerr 2016-09-12 8:55 ` [PATCH 2/4] ipmi: Make ipmi_demangle_device_id more generic Jeremy Kerr 2016-09-12 8:55 ` [PATCH 3/4] ipmi: allow dynamic BMC version information Jeremy Kerr 2016-09-12 16:38 ` [Openipmi-developer] " Corey Minyard 2016-09-13 10:14 ` Jeremy Kerr 2016-09-12 8:55 ` [PATCH 4/4] ipmi/powernv: Use dynamic device ids Jeremy Kerr
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.