| Message ID | 1421290601-3293-1-git-send-email-grimm@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100: > Add reset_loads_image and reset_image_select to sysfs. > > reset_image_select identifies which image will be loaded to the card on the > next PERST. Valid entries are: "user" and "factory". > > reset_loads_image defines functionality on a PERST. Value of 0 means PERST > will not cause image load. A power cycle is required to load the image. Value > of 1 means PERST will cause image load. > > sysfs updates the cxl struct in the driver then calls cxl_update_image_control > to write the vals in the VSEC. Let's combine both of these into a single sysfs file, with "none", "user" and "factory" options and have the show & read functions handle mapping those three options to the two bits in the register. Of the two names I'd probably go with reset_image_select. > +What: /sys/class/cxl/<card>/reset_loads_image > +Date: December 2014 > +Contact: linuxppc-dev@lists.ozlabs.org > +Description: read/write > + Value of 0 means PERST will not cause image load. A power > + cycle is required to load the image. Value of 1 means PERST > + will cause image load. It also seems to be that having this disabled also means that PERST doesn't fully reset the card. Might want to clarify that somewhat and recommend it only be disabled for debugging purposes (e.g. to retain the contents of the PSL trace arrays across a reset), and to always enable it for production. At the moment we don't set it at boot - we just go with whatever the card is already set to do. I'm thinking it might be a good idea to always set this bit on boot so the only time it's disabled is if a user has explicitly gone and disabled it. > +static ssize_t reset_loads_image_show(struct device *device, > + struct device_attribute *attr, > + char *buf) > +{ > + struct cxl *adapter = to_cxl_adapter(device); > + return sprintf(buf, "%d\n", adapter->perst_loads_image); We've used scnprintf for the other sysfs reads in this file, why sprintf here? > +static ssize_t reset_loads_image_store(struct device *device, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct cxl *adapter = to_cxl_adapter(device); > + unsigned long val; > + int rc; > + > + if (kstrtoul(buf, 0, &val) < 0) > + return -EINVAL; > + > + adapter->perst_loads_image = !!val; > + if ((rc = cxl_update_image_control(adapter))) > + return rc; Seems to be some indentation mismatches here - some lines are using spaces other are using tabs. Please use tabs for everything. > +static ssize_t reset_image_select_store(struct device *device, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct cxl *adapter = to_cxl_adapter(device); > + int rc; > + > + if (!strncmp(buf, "user", 4)) > + adapter->perst_select_user = true; > + else if (!strncmp(buf, "factory", 7)) > + adapter->perst_select_user = false; > + else > + return -EINVAL; More indentation mismatches here. Cheers, -Ian
Excerpts from Ian Munsie's message of 2015-01-15 15:41:24 +1100: > At the moment we don't set it at boot - we just go with whatever the > card is already set to do. I'm thinking it might be a good idea to > always set this bit on boot so the only time it's disabled is if a user > has explicitly gone and disabled it. While I think of it - if we change this on boot we should also change reset_image_select to match the currently loaded image. e.g. if reset_loads_image has defaulted to off and reset_image_select has defaulted to factory, but the user image has been loaded - that way we avoid unexpectedly switching to factory if the card gets reset. Cheers, -Ian
> While I think of it - if we change this on boot we should also change > reset_image_select to match the currently loaded image. e.g. if > reset_loads_image has defaulted to off and reset_image_select has > defaulted to factory, but the user image has been loaded - that way we > avoid unexpectedly switching to factory if the card gets reset. Nevermind - I see you have done exactly this in patch 3 :-) -Ian
On Thu, 2015-01-15 at 15:41 +1100, Ian Munsie wrote: > Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100: > > Add reset_loads_image and reset_image_select to sysfs. > > > > reset_image_select identifies which image will be loaded to the card on the > > next PERST. Valid entries are: "user" and "factory". > > > > reset_loads_image defines functionality on a PERST. Value of 0 means PERST > > will not cause image load. A power cycle is required to load the image. Value > > of 1 means PERST will cause image load. > > > > sysfs updates the cxl struct in the driver then calls cxl_update_image_control > > to write the vals in the VSEC. > > Let's combine both of these into a single sysfs file, with "none", > "user" and "factory" options and have the show & read functions handle > mapping those three options to the two bits in the register. > > Of the two names I'd probably go with reset_image_select. Three words, all can be verbs, two can be nouns, it's not too clear. Maybe "load_image_on_perst" ? cheers
Excerpts from Michael Ellerman's message of 2015-01-15 16:07:17 +1100: > > Of the two names I'd probably go with reset_image_select. > > Three words, all can be verbs, two can be nouns, it's not too clear. > > Maybe "load_image_on_perst" ? Works for me :) Cheers, -Ian
Ian, Thanks for reviewing! On 01/14/2015 11:41 PM, Ian Munsie wrote: > Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100: >> Add reset_loads_image and reset_image_select to sysfs. >> >> reset_image_select identifies which image will be loaded to the card on the >> next PERST. Valid entries are: "user" and "factory". >> >> reset_loads_image defines functionality on a PERST. Value of 0 means PERST >> will not cause image load. A power cycle is required to load the image. Value >> of 1 means PERST will cause image load. >> >> sysfs updates the cxl struct in the driver then calls cxl_update_image_control >> to write the vals in the VSEC. > > Let's combine both of these into a single sysfs file, with "none", > "user" and "factory" options and have the show & read functions handle > mapping those three options to the two bits in the register. > I like that idea! > Of the two names I'd probably go with reset_image_select. > >> +What: /sys/class/cxl/<card>/reset_loads_image >> +Date: December 2014 >> +Contact: linuxppc-dev@lists.ozlabs.org >> +Description: read/write >> + Value of 0 means PERST will not cause image load. A power >> + cycle is required to load the image. Value of 1 means PERST >> + will cause image load. > > It also seems to be that having this disabled also means that PERST > doesn't fully reset the card. Might want to clarify that somewhat and > recommend it only be disabled for debugging purposes (e.g. to retain > the contents of the PSL trace arrays across a reset), and to always > enable it for production. > Yeah, that is the main reason you'd disable it. Will add that info to the doc. > At the moment we don't set it at boot - we just go with whatever the > card is already set to do. I'm thinking it might be a good idea to > always set this bit on boot so the only time it's disabled is if a user > has explicitly gone and disabled it. > >> +static ssize_t reset_loads_image_show(struct device *device, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct cxl *adapter = to_cxl_adapter(device); >> + return sprintf(buf, "%d\n", adapter->perst_loads_image); > > We've used scnprintf for the other sysfs reads in this file, why sprintf > here? > No reason, scnprintf is safer so fixed. >> +static ssize_t reset_loads_image_store(struct device *device, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct cxl *adapter = to_cxl_adapter(device); >> + unsigned long val; >> + int rc; >> + >> + if (kstrtoul(buf, 0, &val) < 0) >> + return -EINVAL; >> + >> + adapter->perst_loads_image = !!val; >> + if ((rc = cxl_update_image_control(adapter))) >> + return rc; > > Seems to be some indentation mismatches here - some lines are using > spaces other are using tabs. Please use tabs for everything. > Fixed. >> +static ssize_t reset_image_select_store(struct device *device, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct cxl *adapter = to_cxl_adapter(device); >> + int rc; >> + >> + if (!strncmp(buf, "user", 4)) >> + adapter->perst_select_user = true; >> + else if (!strncmp(buf, "factory", 7)) >> + adapter->perst_select_user = false; >> + else >> + return -EINVAL; > > More indentation mismatches here. > > Fixed. -Ryan > Cheers, > -Ian >
diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl index 554405e..134cfaf 100644 --- a/Documentation/ABI/testing/sysfs-class-cxl +++ b/Documentation/ABI/testing/sysfs-class-cxl @@ -127,3 +127,18 @@ Contact: linuxppc-dev@lists.ozlabs.org Description: read only Will return "user" or "factory" depending on the image loaded onto the card. + +What: /sys/class/cxl/<card>/reset_image_select +Date: December 2014 +Contact: linuxppc-dev@lists.ozlabs.org +Description: read/write + Identifies which image will be loaded to the card on the next + PERST. Valid entries are: "user" and "factory". + +What: /sys/class/cxl/<card>/reset_loads_image +Date: December 2014 +Contact: linuxppc-dev@lists.ozlabs.org +Description: read/write + Value of 0 means PERST will not cause image load. A power + cycle is required to load the image. Value of 1 means PERST + will cause image load. diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h index 0df0438..518c4c6 100644 --- a/drivers/misc/cxl/cxl.h +++ b/drivers/misc/cxl/cxl.h @@ -488,6 +488,7 @@ void cxl_release_one_irq(struct cxl *adapter, int hwirq); int cxl_alloc_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter, unsigned int num); void cxl_release_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter); int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq, unsigned int virq); +int cxl_update_image_control(struct cxl *adapter); /* common == phyp + powernv */ struct cxl_process_element_common { diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c index f801c28..26cacc1 100644 --- a/drivers/misc/cxl/pci.c +++ b/drivers/misc/cxl/pci.c @@ -362,6 +362,41 @@ int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq, return pnv_cxl_ioda_msi_setup(dev, hwirq, virq); } +int cxl_update_image_control(struct cxl *adapter) +{ + struct pci_dev *dev = to_pci_dev(adapter->dev.parent); + int rc; + int vsec; + u8 image_state; + + if (!(vsec = find_cxl_vsec(dev))) { + dev_err(&dev->dev, "ABORTING: CXL VSEC not found!\n"); + return -ENODEV; + } + + if ((rc = CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state))) { + dev_err(&dev->dev, "failed to read image state: %i\n", rc); + return rc; + } + + if (adapter->perst_loads_image) + image_state |= CXL_VSEC_PERST_LOADS_IMAGE; + else + image_state &= ~CXL_VSEC_PERST_LOADS_IMAGE; + + if (adapter->perst_select_user) + image_state |= CXL_VSEC_PERST_SELECT_USER; + else + image_state &= ~CXL_VSEC_PERST_SELECT_USER; + + if ((rc = CXL_WRITE_VSEC_IMAGE_STATE(dev, vsec, image_state))) { + dev_err(&dev->dev, "failed to update image control: %i\n", rc); + return rc; + } + + return 0; +} + int cxl_alloc_one_irq(struct cxl *adapter) { struct pci_dev *dev = to_pci_dev(adapter->dev.parent); diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c index 461bdbd..06f554b 100644 --- a/drivers/misc/cxl/sysfs.c +++ b/drivers/misc/cxl/sysfs.c @@ -56,11 +56,71 @@ static ssize_t image_loaded_show(struct device *device, return scnprintf(buf, PAGE_SIZE, "factory\n"); } +static ssize_t reset_loads_image_show(struct device *device, + struct device_attribute *attr, + char *buf) +{ + struct cxl *adapter = to_cxl_adapter(device); + return sprintf(buf, "%d\n", adapter->perst_loads_image); +} + +static ssize_t reset_loads_image_store(struct device *device, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct cxl *adapter = to_cxl_adapter(device); + unsigned long val; + int rc; + + if (kstrtoul(buf, 0, &val) < 0) + return -EINVAL; + + adapter->perst_loads_image = !!val; + if ((rc = cxl_update_image_control(adapter))) + return rc; + + return count; +} + +static ssize_t reset_image_select_show(struct device *device, + struct device_attribute *attr, + char *buf) +{ + struct cxl *adapter = to_cxl_adapter(device); + + if (adapter->perst_select_user) + return scnprintf(buf, PAGE_SIZE, "user\n"); + + return scnprintf(buf, PAGE_SIZE, "factory\n"); +} + +static ssize_t reset_image_select_store(struct device *device, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct cxl *adapter = to_cxl_adapter(device); + int rc; + + if (!strncmp(buf, "user", 4)) + adapter->perst_select_user = true; + else if (!strncmp(buf, "factory", 7)) + adapter->perst_select_user = false; + else + return -EINVAL; + + if ((rc = cxl_update_image_control(adapter))) + return rc; + + return count; +} + static struct device_attribute adapter_attrs[] = { __ATTR_RO(caia_version), __ATTR_RO(psl_revision), __ATTR_RO(base_image), __ATTR_RO(image_loaded), + __ATTR_RW(reset_loads_image), + __ATTR_RW(reset_image_select), };
Add reset_loads_image and reset_image_select to sysfs. reset_image_select identifies which image will be loaded to the card on the next PERST. Valid entries are: "user" and "factory". reset_loads_image defines functionality on a PERST. Value of 0 means PERST will not cause image load. A power cycle is required to load the image. Value of 1 means PERST will cause image load. sysfs updates the cxl struct in the driver then calls cxl_update_image_control to write the vals in the VSEC. Signed-off-by: Ryan Grimm <grimm@linux.vnet.ibm.com> --- Documentation/ABI/testing/sysfs-class-cxl | 15 ++++++++ drivers/misc/cxl/cxl.h | 1 + drivers/misc/cxl/pci.c | 35 ++++++++++++++++++ drivers/misc/cxl/sysfs.c | 60 +++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+)