[{"id":1764455,"web_url":"http://patchwork.ozlabs.org/comment/1764455/","msgid":"<1504747075.5105.2.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-07T01:17:55","subject":"Re: [PATCH linux dev-4.10 v7 2/2] hwmon: (ucd9000) Add debugfs to\n\tlist mfr_status info","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Wed, 2017-09-06 at 16:00 -0500, Christopher Bostic wrote:\n> Expose via files in debugfs the gpiN_fault fields as well as\n> the entire contents of the mfr_status register.  Allows user\n> space to pull this data for error logging etc...\n> \n> > Signed-off-by: Christopher Bostic \n> ---\n> v7 - Patch order changed from #3 to #2 and as a result needed\n>      to add the ucd9000_remove definition here.\n> v6 - Remove chip idx values on remove so that we can retain the\n>      same indexes regardless how many times we're bound/unbound.\n>    - Remove type 90160 check when adding debugfs.\n> v5 - Add unique debugfs dir ID for each ucd9000 sysfs device.\n>    - Change branching return to a !! method.\n>    - Clean up line breaks, other white space.\n>    - Add comments on assumptions made regarding ucd90160 versus other\n>      types.\n> v2 - Remove mfr_status directory in debugfs and place the files\n>      up in the parent ucd9000 directory.\n> ---\n>  drivers/hwmon/pmbus/ucd9000.c | 166 +++++++++++++++++++++++++++++++++++++++++-\n>  1 file changed, 165 insertions(+), 1 deletion(-)\n> \n> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c\n> index 9a82e88..26870e6 100644\n> --- a/drivers/hwmon/pmbus/ucd9000.c\n> +++ b/drivers/hwmon/pmbus/ucd9000.c\n> @@ -19,6 +19,7 @@\n>   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.\n>   */\n>  \n> +#include \n>  #include \n>  #include \n>  #include \n> @@ -27,6 +28,7 @@\n>  #include \n>  #include \n>  #include \n> +#include \n>  #include \"pmbus.h\"\n>  \n>  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };\n> @@ -35,6 +37,7 @@\n>  #define UCD9000_NUM_PAGES\t\t0xd6\n>  #define UCD9000_FAN_CONFIG_INDEX\t0xe7\n>  #define UCD9000_FAN_CONFIG\t\t0xe8\n> +#define UCD9000_MFR_STATUS\t\t0xf3\n>  #define UCD9000_GPIO_SELECT\t\t0xfa\n>  #define UCD9000_GPIO_CONFIG\t\t0xfb\n>  #define UCD9000_DEVICE_ID\t\t0xfd\n> @@ -56,17 +59,29 @@\n>  #define UCD9000_MON_VOLTAGE_HW\t4\n>  \n>  #define UCD9000_NUM_FAN\t\t4\n> +#define UCD9000_NAME_SIZE\t24\n>  \n>  #define UCD9000_GPIO_NAME_LEN\t16\n>  #define UCD90160_NUM_GPIOS\t26\n> +#define UCD90160_GPI_COUNT\t8\n> +#define UCD90160_GPI_FAULT_BASE\t16\n> +\n> +static DEFINE_IDA(ucd9000_ida);\n>  \n>  struct ucd9000_data {\n>  \tu8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];\n>  \tstruct pmbus_driver_info info;\n>  \tstruct gpio_chip gpio;\n> +\tstruct dentry *debugfs;\n> +\tint idx;\n>  };\n>  #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)\n>  \n> +struct ucd9000_debugfs_entry {\n> +\tstruct i2c_client *client;\n> +\tu8 index;\n> +};\n> +\n>  static int ucd9000_get_fan_config(struct i2c_client *client, int fan)\n>  {\n>  \tint fan_config = 0;\n> @@ -294,6 +309,141 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,\n>  \treturn ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);\n>  }\n>  \n> +static struct dentry *ucd9000_debugfs_dir;\n\nI thought we were fixing this up?\n\n> +\n> +#if IS_ENABLED(CONFIG_DEBUG_FS)\n> +static int ucd9000_get_mfr_status(struct i2c_client *client, u32 *buffer)\n> +{\n> +\tint ret;\n> +\n> +\tret = pmbus_set_page(client, 0);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"pmbus_set_page failed. rc:%d\\n\", ret);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/*\n> +\t * Warning:\n> +\t *\n> +\t * Though not currently supported this will cause stack corruption for\n> +\t * ucd90240!  Command reference, page 81:\n> +\t *\n> +\t *    With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]\n> +\t *    is 2 bytes long (bits 0-15).  With the ucd90240 this command is 5\n> +\t *    bytes long.  With all other devices, it is 4 bytes long.\n> +\t */\n> +\treturn i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS,\n> +\t\t\t\t\t(u8 *)buffer);\n> +}\n> +\n> +static int ucd9000_debugfs_get_mfr_status_bit(void *data, u64 *val)\n> +{\n> +\tstruct ucd9000_debugfs_entry *entry = data;\n> +\tstruct i2c_client *client = entry->client;\n> +\tint nr = entry->index;\n> +\tu32 buffer;\n> +\tint ret;\n> +\n> +\tret = ucd9000_get_mfr_status(client, &buffer);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"Failed to read mfr status. rc:%d\\n\",\n> +\t\t\tret);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t*val = !!(ret & BIT(nr));\n> +\n> +\treturn 0;\n> +}\n> +\n> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_bit,\n> +\t\tucd9000_debugfs_get_mfr_status_bit, NULL, \"%1lld\\n\");\n> +\n> +static int ucd9000_debugfs_get_mfr_status_word(void *data, u64 *val)\n> +{\n> +\tstruct ucd9000_debugfs_entry *entry = data;\n> +\tstruct i2c_client *client = entry->client;\n> +\tu32 buffer;\n> +\tint ret;\n> +\n> +\tret = ucd9000_get_mfr_status(client, &buffer);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"Failed to read mfr status. rc:%d\\n\",\n> +\t\t\tret);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t*val = ret;\n> +\n> +\treturn 0;\n> +}\n> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_word,\n> +\t\tucd9000_debugfs_get_mfr_status_word, NULL, \"%08llx\\n\");\n> +\n> +static int ucd9000_init_debugfs(struct i2c_client *client,\n> +\t\t\t\tstruct ucd9000_data *data)\n> +{\n> +\tstruct ucd9000_debugfs_entry *entries;\n> +\tchar name[UCD9000_NAME_SIZE];\n> +\tint i;\n> +\n> +\tdata->idx = ida_simple_get(&ucd9000_ida, 0, INT_MAX, GFP_KERNEL);\n> +\tscnprintf(name, UCD9000_NAME_SIZE, \"ucd9000.%d\", data->idx);\n> +\tucd9000_debugfs_dir = debugfs_create_dir(name, NULL);\n\nWe should be storing the dentry in the context struct. We're now uniquely\nnaming the directory, but we're still storing the dentry in static storage. The\ncode as it stands will still stomp over the top of existing devices on probe,\nand it cannot be cleaned up properly. In fact, we'll cleanup the wrong entry\ndepending on the order of probe/remove across the devices. It's not just fixed\nby uniquely naming the directory.\n\nAndrew\n\n> +\tif (IS_ERR(ucd9000_debugfs_dir)) {\n> +\t\tdev_warn(&client->dev, \"Failed to create debugfs dir: %p\\n\",\n> +\t\t\tucd9000_debugfs_dir);\n> +\t\tucd9000_debugfs_dir = NULL;\n> +\t\tida_simple_remove(&ucd9000_ida, data->idx);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/*\n> +\t * Warning:\n> +\t *\n> +\t * Makes assumption we're on a ucd90160 type! entries will be different\n> +\t * sizes for other types.\n> +\t */\n> +\tentries = devm_kzalloc(&client->dev, sizeof(*entries) *\n> +\t\t\t\t(UCD90160_GPI_COUNT + 1) * 10, GFP_KERNEL);\n> +\tif (!entries) {\n> +\t\tida_simple_remove(&ucd9000_ida, data->idx);\n> +\t\treturn -ENOMEM;\n> +\t}\n> +\n> +\t/*\n> +\t * Warning:\n> +\t *\n> +\t * This makes the assumption we're probing a ucd90160 type and how the\n> +\t * GPI information is organized.  Needs to account for all other\n> +\t * ucd9000 varieties.\n> +\t */\n> +\tfor (i = 0; i < UCD90160_GPI_COUNT; i++) {\n> +\t\tentries[i].client = client;\n> +\t\tentries[i].index = UCD90160_GPI_FAULT_BASE + i;\n> +\t\tscnprintf(name, UCD9000_NAME_SIZE, \"gpi%d_alarm\", i+1);\n> +\t\tdebugfs_create_file(name, 0444, ucd9000_debugfs_dir,\n> +\t\t\t\t&entries[i],\n> +\t\t\t\t&ucd9000_debugfs_ops_mfr_status_bit);\n> +\t}\n> +\tentries[i].client = client;\n> +\tscnprintf(name, UCD9000_NAME_SIZE, \"mfr_status\");\n> +\tdebugfs_create_file(name, 0444, ucd9000_debugfs_dir, &entries[i],\n> +\t\t\t&ucd9000_debugfs_ops_mfr_status_word);\n> +\n> +\treturn 0;\n> +}\n> +#else\n> +static int ucd9000_init_debugfs(struct i2c_client *client,\n> +\t\t\t\tstruct ucd9000_data *data)\n> +{\n> +\treturn 0;\n> +}\n> +#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */\n> +\n>  static int ucd9000_probe(struct i2c_client *client,\n>  \t\t\t const struct i2c_device_id *id)\n>  {\n> @@ -433,16 +583,30 @@ static int ucd9000_probe(struct i2c_client *client,\n>  \t\treturn ret;\n>  \t}\n>  \n> +\tret = ucd9000_init_debugfs(client, data);\n> +\tif (ret < 0)\n> +\t\tdev_warn(&client->dev, \"Failed to register debugfs: %d\\n\", ret);\n> +\n>  \treturn pmbus_do_probe(client, mid, info);\n>  }\n>  \n> +static int ucd9000_remove(struct i2c_client *client)\n> +{\n> +\tstruct ucd9000_data *data\n> +\t\t= to_ucd9000_data(pmbus_get_driver_info(client));\n> +\n> +\tida_simple_remove(&ucd9000_ida, data->idx);\n> +\tdebugfs_remove_recursive(ucd9000_debugfs_dir);\n> +\treturn pmbus_do_remove(client);\n> +}\n> +\n>  /* This is the driver that will be inserted */\n>  static struct i2c_driver ucd9000_driver = {\n>  \t.driver = {\n>  \t\t.name = \"ucd9000\",\n>  \t},\n>  \t.probe = ucd9000_probe,\n> -\t.remove = pmbus_do_remove,\n> +\t.remove = ucd9000_remove,\n>  \t.id_table = ucd9000_id,\n>  };\n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnjJ63z47z9sCZ\n\tfor ;\n\tThu, 7 Sep 2017 11:18:14 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xnjJ62hZmzDrWF\n\tfor ;\n\tThu, 7 Sep 2017 11:18:14 +1000 (AEST)","from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com\n\t[66.111.4.25])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xnjHx68QfzDrSL\n\tfor ; Thu, 7 Sep 2017 11:18:05 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 3789020C10;\n\tWed, 6 Sep 2017 21:18:03 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Wed, 06 Sep 2017 21:18:03 -0400","from keelia (bh02i525f01.au.ibm.com [202.81.18.30])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 7C47924A60;\n\tWed, 6 Sep 2017 21:18:01 -0400 (EDT)"],"Authentication-Results":["ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"EPPBjqqo\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"A2MF4spw\"; \n\tdkim-atps=neutral","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"EPPBjqqo\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"A2MF4spw\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.25; helo=out1-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"EPPBjqqo\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"A2MF4spw\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=6Pb0qgHeny8nWGAzP1ekHfh/phbxBJKXZ6VYIcXxT\n\tgg=; b=EPPBjqqonAw4lG2/hRoFNZs4QhSCyuxGonuXxBBtZBhOm3HvSpWuOxasK\n\ttjiUTxBF1OHct0QCc1XeS1+t76lCZscJGQEipmrQXWyBcObFMOCgvbKBcoDgocsm\n\t9MnodsdNMLwoSWzsWVqpeAJ7qNkg8Ec4y5BVRGKefNJl4+7sdiwqrpBqfjcvzsrt\n\t7569TmuLZM8qQvs6xKFMBTG2NwaItdAMAGz8yUG6sthVHY0qsrBxRILu8vtlahgU\n\tYG9sIZoyIQkTnuEz99VZR/aTZYOfFyvo1vH/8GZFG4CFkeiPNO3W1Ryl+IM0iv6K\n\t6f4kz0a6USD3Hmyz+B0d5Sw9ylIOg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=6Pb0qgHeny8nWGAzP1\n\tekHfh/phbxBJKXZ6VYIcXxTgg=; b=A2MF4spww9D41SPMMyGZaXxHqUThw/b6SY\n\t7Yg6+lpSAjOAVvPGx4foJLL9GEL1ev4zBKmL8rGAYBMUHCPU42LxAXT43GnEjtRS\n\tB9EAGTZuydKMYhLxgohi2bnVor0okvI5Y2TaWVNoWkmRdnw9IEfxCWWds2l/+yB4\n\tWa2ZhgPY3dIUsL1zHqZl7AykizJaZmstYFotYsyNpdrG029KaCR7iS21iJr9V9D1\n\tJgUUxf3wlwoT1m3ExL34Zmlr78M3B1vBPk4DqDg0vczt4LfnK4VvVZ1IVcHfG9Hq\n\tIgbjpK9X1Hj16B9CnhdIxGoWGzgnS0HOngDNb7WvO8RW7dJggfJQ=="],"X-ME-Sender":"","X-Sasl-enc":"plgW2yc1UTXEvy/hmPxkqS20LcdqGzRrC4prdDZG9h1L 1504747082","Message-ID":"<1504747075.5105.2.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 v7 2/2] hwmon: (ucd9000) Add debugfs to\n\tlist mfr_status info","From":"Andrew Jeffery ","To":"Christopher Bostic , joel@jms.id.au","Date":"Thu, 07 Sep 2017 11:17:55 +1000","In-Reply-To":"<20170906210041.22533-3-cbostic@linux.vnet.ibm.com>","References":"<20170906210041.22533-1-cbostic@linux.vnet.ibm.com>\n\t<20170906210041.22533-3-cbostic@linux.vnet.ibm.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-K/F3yY/TXHSZmphcOeCA\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"openbmc@lists.ozlabs.org","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1764855,"web_url":"http://patchwork.ozlabs.org/comment/1764855/","msgid":"","list_archive_url":null,"date":"2017-09-07T17:43:32","subject":"Re: [PATCH linux dev-4.10 v7 2/2] hwmon: (ucd9000) Add debugfs to\n\tlist mfr_status info","submitter":{"id":70879,"url":"http://patchwork.ozlabs.org/api/people/70879/","name":"Christopher Bostic","email":"cbostic@linux.vnet.ibm.com"},"content":"On 9/6/17 8:17 PM, Andrew Jeffery wrote:\n> On Wed, 2017-09-06 at 16:00 -0500, Christopher Bostic wrote:\n>> Expose via files in debugfs the gpiN_fault fields as well as\n>> the entire contents of the mfr_status register. Allows user\n>> space to pull this data for error logging etc...\n>> \n>>> Signed-off-by: Christopher Bostic \n>> ---\n>> v7 - Patch order changed from #3 to #2 and as a result needed\n>> to add the ucd9000_remove definition here.\n>> v6 - Remove chip idx values on remove so that we can retain the\n>> same indexes regardless how many times we're bound/unbound.\n>> - Remove type 90160 check when adding debugfs.\n>> v5 - Add unique debugfs dir ID for each ucd9000 sysfs device.\n>> - Change branching return to a !! method.\n>> - Clean up line breaks, other white space.\n>> - Add comments on assumptions made regarding ucd90160 versus other\n>> types.\n>> v2 - Remove mfr_status directory in debugfs and place the files\n>> up in the parent ucd9000 directory.\n>> ---\n>> drivers/hwmon/pmbus/ucd9000.c | 166 +++++++++++++++++++++++++++++++++++++++++-\n>> 1 file changed, 165 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c\n>> index 9a82e88..26870e6 100644\n>> --- a/drivers/hwmon/pmbus/ucd9000.c\n>> +++ b/drivers/hwmon/pmbus/ucd9000.c\n>> @@ -19,6 +19,7 @@\n>> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.\n>> */\n>> \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -27,6 +28,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \"pmbus.h\"\n>> \n>> enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };\n>> @@ -35,6 +37,7 @@\n>> #define UCD9000_NUM_PAGES\t\t0xd6\n>> #define UCD9000_FAN_CONFIG_INDEX\t0xe7\n>> #define UCD9000_FAN_CONFIG\t\t0xe8\n>> +#define UCD9000_MFR_STATUS\t\t0xf3\n>> #define UCD9000_GPIO_SELECT\t\t0xfa\n>> #define UCD9000_GPIO_CONFIG\t\t0xfb\n>> #define UCD9000_DEVICE_ID\t\t0xfd\n>> @@ -56,17 +59,29 @@\n>> #define UCD9000_MON_VOLTAGE_HW\t4\n>> \n>> #define UCD9000_NUM_FAN\t\t4\n>> +#define UCD9000_NAME_SIZE\t24\n>> \n>> #define UCD9000_GPIO_NAME_LEN\t16\n>> #define UCD90160_NUM_GPIOS\t26\n>> +#define UCD90160_GPI_COUNT\t8\n>> +#define UCD90160_GPI_FAULT_BASE\t16\n>> +\n>> +static DEFINE_IDA(ucd9000_ida);\n>> \n>> struct ucd9000_data {\n>> \tu8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];\n>> \tstruct pmbus_driver_info info;\n>> \tstruct gpio_chip gpio;\n>> +\tstruct dentry *debugfs;\n>> +\tint idx;\n>> };\n>> #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)\n>> \n>> +struct ucd9000_debugfs_entry {\n>> +\tstruct i2c_client *client;\n>> +\tu8 index;\n>> +};\n>> +\n>> static int ucd9000_get_fan_config(struct i2c_client *client, int fan)\n>> {\n>> \tint fan_config = 0;\n>> @@ -294,6 +309,141 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,\n>> \treturn ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);\n>> }\n>> \n>> +static struct dentry *ucd9000_debugfs_dir;\n> I thought we were fixing this up?\n>\n>> +\n>> +#if IS_ENABLED(CONFIG_DEBUG_FS)\n>> +static int ucd9000_get_mfr_status(struct i2c_client *client, u32 *buffer)\n>> +{\n>> +\tint ret;\n>> +\n>> +\tret = pmbus_set_page(client, 0);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"pmbus_set_page failed. rc:%d\\n\", ret);\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Warning:\n>> +\t *\n>> +\t * Though not currently supported this will cause stack corruption for\n>> +\t * ucd90240! Command reference, page 81:\n>> +\t *\n>> +\t * With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]\n>> +\t * is 2 bytes long (bits 0-15). With the ucd90240 this command is 5\n>> +\t * bytes long. With all other devices, it is 4 bytes long.\n>> +\t */\n>> +\treturn i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS,\n>> +\t\t\t\t\t(u8 *)buffer);\n>> +}\n>> +\n>> +static int ucd9000_debugfs_get_mfr_status_bit(void *data, u64 *val)\n>> +{\n>> +\tstruct ucd9000_debugfs_entry *entry = data;\n>> +\tstruct i2c_client *client = entry->client;\n>> +\tint nr = entry->index;\n>> +\tu32 buffer;\n>> +\tint ret;\n>> +\n>> +\tret = ucd9000_get_mfr_status(client, &buffer);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"Failed to read mfr status. rc:%d\\n\",\n>> +\t\t\tret);\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\t*val = !!(ret & BIT(nr));\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_bit,\n>> +\t\tucd9000_debugfs_get_mfr_status_bit, NULL, \"%1lld\\n\");\n>> +\n>> +static int ucd9000_debugfs_get_mfr_status_word(void *data, u64 *val)\n>> +{\n>> +\tstruct ucd9000_debugfs_entry *entry = data;\n>> +\tstruct i2c_client *client = entry->client;\n>> +\tu32 buffer;\n>> +\tint ret;\n>> +\n>> +\tret = ucd9000_get_mfr_status(client, &buffer);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"Failed to read mfr status. rc:%d\\n\",\n>> +\t\t\tret);\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\t*val = ret;\n>> +\n>> +\treturn 0;\n>> +}\n>> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_word,\n>> +\t\tucd9000_debugfs_get_mfr_status_word, NULL, \"%08llx\\n\");\n>> +\n>> +static int ucd9000_init_debugfs(struct i2c_client *client,\n>> +\t\t\t\tstruct ucd9000_data *data)\n>> +{\n>> +\tstruct ucd9000_debugfs_entry *entries;\n>> +\tchar name[UCD9000_NAME_SIZE];\n>> +\tint i;\n>> +\n>> +\tdata->idx = ida_simple_get(&ucd9000_ida, 0, INT_MAX, GFP_KERNEL);\n>> +\tscnprintf(name, UCD9000_NAME_SIZE, \"ucd9000.%d\", data->idx);\n>> +\tucd9000_debugfs_dir = debugfs_create_dir(name, NULL);\n> We should be storing the dentry in the context struct. We're now uniquely\n> naming the directory, but we're still storing the dentry in static storage. The\n> code as it stands will still stomp over the top of existing devices on probe,\n> and it cannot be cleaned up properly. In fact, we'll cleanup the wrong entry\n> depending on the order of probe/remove across the devices. It's not just fixed\n> by uniquely naming the directory.\n\nOK, will correct for next set.\n\nThanks,\nChris\n\n>\n> Andrew\n>\n>> +\tif (IS_ERR(ucd9000_debugfs_dir)) {\n>> +\t\tdev_warn(&client->dev, \"Failed to create debugfs dir: %p\\n\",\n>> +\t\t\tucd9000_debugfs_dir);\n>> +\t\tucd9000_debugfs_dir = NULL;\n>> +\t\tida_simple_remove(&ucd9000_ida, data->idx);\n>> +\t\treturn 0;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Warning:\n>> +\t *\n>> +\t * Makes assumption we're on a ucd90160 type! entries will be different\n>> +\t * sizes for other types.\n>> +\t */\n>> +\tentries = devm_kzalloc(&client->dev, sizeof(*entries) *\n>> +\t\t\t\t(UCD90160_GPI_COUNT + 1) * 10, GFP_KERNEL);\n>> +\tif (!entries) {\n>> +\t\tida_simple_remove(&ucd9000_ida, data->idx);\n>> +\t\treturn -ENOMEM;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Warning:\n>> +\t *\n>> +\t * This makes the assumption we're probing a ucd90160 type and how the\n>> +\t * GPI information is organized. Needs to account for all other\n>> +\t * ucd9000 varieties.\n>> +\t */\n>> +\tfor (i = 0; i < UCD90160_GPI_COUNT; i++) {\n>> +\t\tentries[i].client = client;\n>> +\t\tentries[i].index = UCD90160_GPI_FAULT_BASE + i;\n>> +\t\tscnprintf(name, UCD9000_NAME_SIZE, \"gpi%d_alarm\", i+1);\n>> +\t\tdebugfs_create_file(name, 0444, ucd9000_debugfs_dir,\n>> +\t\t\t\t&entries[i],\n>> +\t\t\t\t&ucd9000_debugfs_ops_mfr_status_bit);\n>> +\t}\n>> +\tentries[i].client = client;\n>> +\tscnprintf(name, UCD9000_NAME_SIZE, \"mfr_status\");\n>> +\tdebugfs_create_file(name, 0444, ucd9000_debugfs_dir, &entries[i],\n>> +\t\t\t&ucd9000_debugfs_ops_mfr_status_word);\n>> +\n>> +\treturn 0;\n>> +}\n>> +#else\n>> +static int ucd9000_init_debugfs(struct i2c_client *client,\n>> +\t\t\t\tstruct ucd9000_data *data)\n>> +{\n>> +\treturn 0;\n>> +}\n>> +#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */\n>> +\n>> static int ucd9000_probe(struct i2c_client *client,\n>> \t\t\t const struct i2c_device_id *id)\n>> {\n>> @@ -433,16 +583,30 @@ static int ucd9000_probe(struct i2c_client *client,\n>> \t\treturn ret;\n>> \t}\n>> \n>> +\tret = ucd9000_init_debugfs(client, data);\n>> +\tif (ret < 0)\n>> +\t\tdev_warn(&client->dev, \"Failed to register debugfs: %d\\n\", ret);\n>> +\n>> \treturn pmbus_do_probe(client, mid, info);\n>> }\n>> \n>> +static int ucd9000_remove(struct i2c_client *client)\n>> +{\n>> +\tstruct ucd9000_data *data\n>> +\t\t= to_ucd9000_data(pmbus_get_driver_info(client));\n>> +\n>> +\tida_simple_remove(&ucd9000_ida, data->idx);\n>> +\tdebugfs_remove_recursive(ucd9000_debugfs_dir);\n>> +\treturn pmbus_do_remove(client);\n>> +}\n>> +\n>> /* This is the driver that will be inserted */\n>> static struct i2c_driver ucd9000_driver = {\n>> \t.driver = {\n>> \t\t.name = \"ucd9000\",\n>> \t},\n>> \t.probe = ucd9000_probe,\n>> -\t.remove = pmbus_do_remove,\n>> +\t.remove = ucd9000_remove,\n>> \t.id_table = ucd9000_id,\n>> };\n>>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xp79N4vNgz9s83\n\tfor ;\n\tFri, 8 Sep 2017 03:43:52 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xp79N3XRPzDrY5\n\tfor ;\n\tFri, 8 Sep 2017 03:43:52 +1000 (AEST)","from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com\n\t[148.163.158.5])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xp79C3ZNTzDrWV\n\tfor ; Fri, 8 Sep 2017 03:43:43 +1000 (AEST)","from pps.filterd (m0098417.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv87Hd6BM033032\n\tfor ; Thu, 7 Sep 2017 13:43:40 -0400","from e32.co.us.ibm.com (e32.co.us.ibm.com [32.97.110.150])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2cua3rj38w-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Thu, 07 Sep 2017 13:43:40 -0400","from localhost\n\tby e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tThu, 7 Sep 2017 11:43:39 -0600","from b03cxnp08025.gho.boulder.ibm.com (9.17.130.17)\n\tby e32.co.us.ibm.com (192.168.1.132) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tThu, 7 Sep 2017 11:43:37 -0600","from b03ledav006.gho.boulder.ibm.com\n\t(b03ledav006.gho.boulder.ibm.com [9.17.130.237])\n\tby b03cxnp08025.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v87HhWHa2753022; Thu, 7 Sep 2017 10:43:36 -0700","from b03ledav006.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id AA1C6C6042;\n\tThu, 7 Sep 2017 11:43:36 -0600 (MDT)","from christophersmbp.austin.ibm.com (unknown [9.83.1.167])\n\tby b03ledav006.gho.boulder.ibm.com (Postfix) with ESMTP id B5B38C6043;\n\tThu, 7 Sep 2017 11:43:34 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=cbostic@linux.vnet.ibm.com; receiver=)","Subject":"Re: [PATCH linux dev-4.10 v7 2/2] hwmon: (ucd9000) Add debugfs to\n\tlist mfr_status info","To":"Andrew Jeffery , joel@jms.id.au","References":"<20170906210041.22533-1-cbostic@linux.vnet.ibm.com>\n\t<20170906210041.22533-3-cbostic@linux.vnet.ibm.com>\n\t<1504747075.5105.2.camel@aj.id.au>","From":"Christopher Bostic ","Date":"Thu, 7 Sep 2017 12:43:32 -0500","User-Agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0)\n\tGecko/20100101 Thunderbird/45.7.0","MIME-Version":"1.0","In-Reply-To":"<1504747075.5105.2.camel@aj.id.au>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","X-TM-AS-GCONF":"00","x-cbid":"17090717-0004-0000-0000-000012E2F642","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007684; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000226; SDB=6.00913651; UDB=6.00458580;\n\tIPR=6.00693909; \n\tBA=6.00005576; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017054;\n\tXFM=3.00000015; UTC=2017-09-07 17:43:38","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17090717-0005-0000-0000-0000810611BB","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-07_10:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709070261","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"openbmc@lists.ozlabs.org","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}}]