From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp05.au.ibm.com (e23smtp05.au.ibm.com [202.81.31.147]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id CC7821A0D63 for ; Fri, 16 Jan 2015 15:27:30 +1100 (AEDT) Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 16 Jan 2015 14:27:29 +1000 Received: from d23relay09.au.ibm.com (d23relay09.au.ibm.com [9.185.63.181]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 97D5C2BB004D for ; Fri, 16 Jan 2015 15:27:27 +1100 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay09.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t0G4RRO738666336 for ; Fri, 16 Jan 2015 15:27:27 +1100 Received: from d23av01.au.ibm.com (localhost [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t0G4RQlL027388 for ; Fri, 16 Jan 2015 15:27:27 +1100 Content-Type: text/plain; charset=UTF-8 From: Ian Munsie To: Ryan Grimm Subject: Re: [PATCH 3/3] CXL: Add ability to reset the card In-reply-to: <1421360837-40287-3-git-send-email-grimm@linux.vnet.ibm.com> References: <1421360837-40287-1-git-send-email-grimm@linux.vnet.ibm.com> <1421360837-40287-3-git-send-email-grimm@linux.vnet.ibm.com> Date: Fri, 16 Jan 2015 15:27:26 +1100 Message-Id: <1421381813-sup-3338@delenn.ozlabs.ibm.com> Cc: mikey , linuxppc-dev List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Ryan, Excerpts from Ryan Grimm's message of 2015-01-16 09:27:17 +1100: > Adds reset to sysfs which will PERST the card. If load_image_on_perst is set > to "user" or "factory", the PERST will cause that image to be loaded. > > load_image_on_perst is set to "user" for production. While it generally will be "user" for production, some cards may only ship with only a factory image, which is then going to be the image used for production. It might be better to say that it will default to whichever image the card has already loaded. > Value of "none" means PERST will not cause image to be loaded > - to the card. A power cycle is required to load the image. > + to the card. A power cycle is required to load the image. > + "none" is useful for debugging so the contents of the trace > + arrays are preserved. > Value of "user" and "factory" means PERST will cause either the > - user or factory image to be loaded. > + user or factory image to be loaded. "user" is default and > + should be used in production. git am spotted some whitespace at the end of a couple of lines here > + /* pcie_warm_reset requests a fundamental pci reset which includes a > + * PERST assert/deassert. PERST triggers a loading of the image > + * if "user" or "factory" is selected in sysfs */ > + if ((rc = pci_set_pcie_reset_state(dev, pcie_warm_reset))) { > + dev_err(&dev->dev, "cxl: pcie_warm_reset failed\n"); > + return rc; > + } > + > + /* the PERST done above fences the PHB. So, reset depends on EEH > + * to unbind the driver, tell Sapphire to reinit the PHB, and rebind > + * the driver. Do an mmio read explictly to ensure EEH notices the > + * fenced PHB. Retry for a few seconds before giving up. */ Great, thanks for adding the explanations here :) > @@ -806,8 +843,8 @@ static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev) > CXL_READ_VSEC_BASE_IMAGE(dev, vsec, &adapter->base_image); > CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state); > adapter->user_image_loaded = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED); > - adapter->perst_loads_image = !!(image_state & CXL_VSEC_PERST_LOADS_IMAGE); > - adapter->perst_select_user = !!(image_state & CXL_VSEC_PERST_SELECT_USER); > + adapter->perst_loads_image = true; > + adapter->perst_select_user = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED); > + if ((rc = cxl_update_image_control(adapter))) > + goto err2; Please move these two hunks into a separate patch (can be first in the series) along with the cxl_update_image_control() function from patch 1 - I'd like to get this backported to stable, which will be simpler if it is in it's own patch. > +static ssize_t reset_adapter_store(struct device *device, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct cxl *adapter = to_cxl_adapter(device); > + int rc; > + > + if ((rc = cxl_reset(adapter))) > + return rc; > + return count; > +} Please add a check here so the reset only occurs when a "1" is written to this file to match the documentation. Cheers, -Ian