Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Zev Weiss <zweiss@equinix.com>
To:     Andrew Jeffery <andrew@aj.id.au>
CC:     "openipmi-developer@lists.sourceforge.net" 
        <openipmi-developer@lists.sourceforge.net>,
        "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
        Corey Minyard <minyard@acm.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Ryan Chen <ryan_chen@aspeedtech.com>,
        Tomer Maimon <tmaimon77@gmail.com>,
        "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
        Avi Fishman <avifishman70@gmail.com>,
        Patrick Venture <venture@google.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Tali Perry <tali.perry1@gmail.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Lee Jones <lee.jones@linaro.org>,
        "Chia-Wei, Wang" <chiawei_wang@aspeedtech.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        Benjamin Fair <benjaminfair@google.com>
Subject: Re: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures
 inside-out
Thread-Topic: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures
 inside-out
Thread-Index: AQHXLQkhK5HJQiDBlUq3BrfCbbMJraqskkSA
Date:   Fri, 9 Apr 2021 19:26:00 +0000
Message-ID: <YHCqR8/nZFB1HRgX@packtop>
References: <20210319062752.145730-1-andrew@aj.id.au>
 <20210319062752.145730-10-andrew@aj.id.au> <YG/Ql9z9X/mtOSvl@packtop>
 <ea34ee69-a266-4737-8450-4695d2d0fbd4@www.fastmail.com>
 <YG/zVv4XOo1HoLd1@packtop>
In-Reply-To: <YG/zVv4XOo1HoLd1@packtop>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <0DAB7C08AE0DF741847CE60B21A5D795@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 8355b03a-498a-4895-2615-08d8fb8d4ebd
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Apr 2021 19:26:00.2124
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72adb271-2fc7-4afe-a5ee-9de6a59f6bfb
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: iiZLdwP9cqFxPAfMLUoeyePf5d3s37lHYReZhFTgCF8zDJhbBVZHcA32tm+bdOjdhQtPyMQNkzPHJv3D7BwKJA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR04MB0363
X-Proofpoint-GUID: vh-OeKhVXl2akBm7y0TxbfBSl3kKteAe
X-Proofpoint-ORIG-GUID: vh-OeKhVXl2akBm7y0TxbfBSl3kKteAe
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761
 definitions=2021-04-09_07:2021-04-09,2021-04-09 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 malwarescore=0
 suspectscore=0 mlxlogscore=999 impostorscore=0 bulkscore=0 clxscore=1015
 lowpriorityscore=0 adultscore=0 priorityscore=1501 mlxscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104060000
 definitions=main-2104090139
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Apr 09, 2021 at 01:25:26AM CDT, Zev Weiss wrote:
>On Fri, Apr 09, 2021 at 12:59:09AM CDT, Andrew Jeffery wrote:
>>
>>
>>On Fri, 9 Apr 2021, at 13:27, Zev Weiss wrote:
>>>On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote:
>>>>Make the KCS device drivers responsible for allocating their own memory=
.
>>>>
>>>>Until now the private data for the device driver was allocated internal
>>>>to the private data for the chardev interface. This coupling required
>>>>the slightly awkward API of passing through the struct size for the
>>>>driver private data to the chardev constructor, and then retrieving a
>>>>pointer to the driver private data from the allocated chardev memory.
>>>>
>>>>In addition to being awkward, the arrangement prevents the
>>>>implementation of alternative userspace interfaces as the device driver
>>>>private data is not independent.
>>>>
>>>>Peel a layer off the onion and turn the data-structures inside out by
>>>>exploiting container_of() and embedding `struct kcs_device` in the
>>>>driver private data.
>>>>
>>>>Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>>>---
>>>> drivers/char/ipmi/kcs_bmc.c           | 15 +++++--
>>>> drivers/char/ipmi/kcs_bmc.h           | 12 ++----
>>>> drivers/char/ipmi/kcs_bmc_aspeed.c    | 60 ++++++++++++++++-----------
>>>> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++++++++++++++++++---------
>>>> drivers/char/ipmi/kcs_bmc_npcm7xx.c   | 37 ++++++++++-------
>>>> 5 files changed, 113 insertions(+), 71 deletions(-)
>>>>
>>>>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>>>>index ef5c48ffe74a..709b6bdec165 100644
>>>>--- a/drivers/char/ipmi/kcs_bmc.c
>>>>+++ b/drivers/char/ipmi/kcs_bmc.c
>>>>@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
>>>> }
>>>> EXPORT_SYMBOL(kcs_bmc_handle_event);
>>>>
>>>>-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv=
, u32 channel);
>>>>-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32=
 channel)
>>>>+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
>>>
>>>Another declaration perhaps intended for kcs_bmc.h?
>>
>>These are temporary while the code gets shuffled around. The symbol
>>name is an implementation detail, not a "public" part of the API; after
>>some further shuffling these are eventually assigned as callbacks in an
>>ops struct.
>>
>
>Okay, that makes sense.
>
>>>
>>>>+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
>>>> {
>>>>-	return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
>>>>+	return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
>>>> }
>>>>-EXPORT_SYMBOL(kcs_bmc_alloc);
>>>>+EXPORT_SYMBOL(kcs_bmc_add_device);
>>>>+
>>>>+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
>>>
>>>Here too.
>>>
>>>>+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
>>>>+{
>>>>+	return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
>>>>+}
>>>>+EXPORT_SYMBOL(kcs_bmc_remove_device);
>>>>
>>>> MODULE_LICENSE("GPL v2");
>>>> MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
>>>>diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
>>>>index febea0c8deb4..bf0ae327997f 100644
>>>>--- a/drivers/char/ipmi/kcs_bmc.h
>>>>+++ b/drivers/char/ipmi/kcs_bmc.h
>>>>@@ -67,6 +67,8 @@ struct kcs_ioreg {
>>>> };
>>>>
>>>> struct kcs_bmc {
>>>>+	struct device *dev;
>>>>+
>>>> 	spinlock_t lock;
>>>>
>>>> 	u32 channel;
>>>>@@ -94,17 +96,11 @@ struct kcs_bmc {
>>>> 	u8 *kbuffer;
>>>>
>>>> 	struct miscdevice miscdev;
>>>>-
>>>>-	unsigned long priv[];
>>>> };
>>>>
>>>>-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
>>>>-{
>>>>-	return kcs_bmc->priv;
>>>>-}
>>>>-
>>>> int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
>>>>-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32=
 channel);
>>>>+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
>>>>+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
>>>>
>>>> u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
>>>> void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
>>>>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs=
_bmc_aspeed.c
>>>>index 630cf095560e..0416ac78ce68 100644
>>>>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>>>>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>>>>@@ -61,6 +61,8 @@
>>>> #define LPC_STR4             0x11C
>>>>
>>>> struct aspeed_kcs_bmc {
>>>>+	struct kcs_bmc kcs_bmc;
>>>>+
>>>> 	struct regmap *map;
>>>> };
>>>>
>>>>@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
>>>> 	int (*get_io_address)(struct platform_device *pdev);
>>>> };
>>>>
>>>>+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc =
*kcs_bmc)
>>>>+{
>>>>+	return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
>>>>+}
>>>>+
>>>> static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>>>> {
>>>>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>>>>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
>>>> 	u32 val =3D 0;
>>>> 	int rc;
>>>>
>>>>@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32=
 reg)
>>>>
>>>> static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
>>>> {
>>>>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>>>>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
>>>> 	int rc;
>>>>
>>>> 	rc =3D regmap_write(priv->map, reg, data);
>>>>@@ -92,7 +99,7 @@ static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, =
u32 reg, u8 data)
>>>>
>>>> static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 ma=
sk, u8 val)
>>>> {
>>>>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>>>>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
>>>> 	int rc;
>>>>
>>>> 	rc =3D regmap_update_bits(priv->map, reg, mask, val);
>>>>@@ -114,7 +121,7 @@ static void aspeed_kcs_updateb(struct kcs_bmc *kcs_=
bmc, u32 reg, u8 mask, u8 val
>>>>  */
>>>> static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
>>>> {
>>>>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>>>>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
>>>>
>>>> 	switch (kcs_bmc->channel) {
>>>> 	case 1:
>>>>@@ -148,7 +155,7 @@ static void aspeed_kcs_set_address(struct kcs_bmc *=
kcs_bmc, u16 addr)
>>>>
>>>> static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool en=
able)
>>>> {
>>>>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>>>>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
>>>>
>>>> 	switch (kcs_bmc->channel) {
>>>> 	case 1:
>>>>@@ -323,16 +330,16 @@ static int aspeed_kcs_of_v2_get_io_address(struct=
 platform_device *pdev)
>>>> static int aspeed_kcs_probe(struct platform_device *pdev)
>>>> {
>>>> 	const struct aspeed_kcs_of_ops *ops;
>>>>-	struct device *dev =3D &pdev->dev;
>>>>+	struct aspeed_kcs_bmc *priv;
>>>> 	struct kcs_bmc *kcs_bmc;
>>>> 	struct device_node *np;
>>>> 	int rc, channel, addr;
>>>>
>>>>-	np =3D dev->of_node->parent;
>>>>+	np =3D pdev->dev.of_node->parent;
>>>> 	if (!of_device_is_compatible(np, "aspeed,ast2400-lpc-v2") &&
>>>> 	    !of_device_is_compatible(np, "aspeed,ast2500-lpc-v2") &&
>>>> 	    !of_device_is_compatible(np, "aspeed,ast2600-lpc-v2")) {
>>>>-		dev_err(dev, "unsupported LPC device binding\n");
>>>>+		dev_err(&pdev->dev, "unsupported LPC device binding\n");
>>>> 		return -ENODEV;
>>>> 	}
>>>> 	ops =3D of_device_get_match_data(&pdev->dev);
>>>>@@ -343,18 +350,27 @@ static int aspeed_kcs_probe(struct platform_devic=
e *pdev)
>>>> 	if (channel < 0)
>>>> 		return channel;
>>>>
>>>>-	kcs_bmc =3D kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), =
channel);
>>>>-	if (!kcs_bmc)
>>>>+	addr =3D ops->get_io_address(pdev);
>>>>+	if (addr < 0)
>>>>+		return addr;
>>>>+
>>>>+	priv =3D devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>>+	if (!priv)
>>>> 		return -ENOMEM;
>>>>
>>>>+	kcs_bmc =3D &priv->kcs_bmc;
>>>>+	kcs_bmc->dev =3D &pdev->dev;
>>>>+	kcs_bmc->channel =3D channel;
>>>> 	kcs_bmc->ioreg =3D ast_kcs_bmc_ioregs[channel - 1];
>>>> 	kcs_bmc->io_inputb =3D aspeed_kcs_inb;
>>>> 	kcs_bmc->io_outputb =3D aspeed_kcs_outb;
>>>> 	kcs_bmc->io_updateb =3D aspeed_kcs_updateb;
>>>>
>>>>-	addr =3D ops->get_io_address(pdev);
>>>>-	if (addr < 0)
>>>>-		return addr;
>>>>+	priv->map =3D syscon_node_to_regmap(pdev->dev.parent->of_node);
>>>>+	if (IS_ERR(priv->map)) {
>>>>+		dev_err(&pdev->dev, "Couldn't get regmap\n");
>>>>+		return -ENODEV;
>>>>+	}
>>>
>>>The reanimated priv->map initialization I suspect wasn't meant to
>>>have been removed in the first place...
>>
>>Yeah, I'll have to go back and figure out what went wrong there!
>>
>>Thanks for catching that.
>>
>>>
>>>>
>>>> 	aspeed_kcs_set_address(kcs_bmc, addr);
>>>>
>>>>@@ -362,29 +378,25 @@ static int aspeed_kcs_probe(struct platform_devic=
e *pdev)
>>>> 	if (rc)
>>>> 		return rc;
>>>>
>>>>-	dev_set_drvdata(dev, kcs_bmc);
>>>>+	platform_set_drvdata(pdev, priv);
>>>>
>>>> 	aspeed_kcs_enable_channel(kcs_bmc, true);
>>>>
>>>>-	rc =3D misc_register(&kcs_bmc->miscdev);
>>>>-	if (rc) {
>>>>-		dev_err(dev, "Unable to register device\n");
>>>>+	rc =3D kcs_bmc_add_device(&priv->kcs_bmc);
>>>>+	if (rc < 0)
>>>> 		return rc;
>>>>-	}
>>>>
>>>>-	dev_dbg(&pdev->dev,
>>>>-		"Probed KCS device %d (IDR=3D0x%x, ODR=3D0x%x, STR=3D0x%x)\n",
>>>>-		kcs_bmc->channel, kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr,
>>>>-		kcs_bmc->ioreg.str);
>>>>+	dev_info(&pdev->dev, "Initialised channel %d at 0x%x\n", kcs_bmc->cha=
nnel, addr);
>>>
>>>Is the dbg->info change here intentional?  (I have no particular
>>>objection if so, but it's often a change I make myself during
>>>testing/debugging and then forget to revert...)
>>
>>Yeah, it was possibly something I forgot to revert. If others have
>>issues with it staying at dev_info() I'll switch it back.
>>
>>>
>>>>
>>>> 	return 0;
>>>> }
>>>>
>>>> static int aspeed_kcs_remove(struct platform_device *pdev)
>>>> {
>>>>-	struct kcs_bmc *kcs_bmc =3D dev_get_drvdata(&pdev->dev);
>>>>+	struct aspeed_kcs_bmc *priv =3D platform_get_drvdata(pdev);
>>>>+	struct kcs_bmc *kcs_bmc =3D &priv->kcs_bmc;
>>>>
>>>>-	misc_deregister(&kcs_bmc->miscdev);
>>>>+	kcs_bmc_remove_device(kcs_bmc);
>>>
>>>Should we propagate the return value outward here?
>>
>>Probably!
>>
>>>
>>>>
>>>> 	return 0;
>>>> }
>>>>diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/=
kcs_bmc_cdev_ipmi.c
>>>>index 82c77994e481..0ca71c135a1a 100644
>>>>--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
>>>>+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
>>>>@@ -382,7 +382,7 @@ static int kcs_bmc_ipmi_release(struct inode *inode=
, struct file *filp)
>>>> 	return 0;
>>>> }
>>>>
>>>>-static const struct file_operations kcs_bmc_fops =3D {
>>>>+static const struct file_operations kcs_bmc_ipmi_fops =3D {
>>>> 	.owner          =3D THIS_MODULE,
>>>> 	.open           =3D kcs_bmc_ipmi_open,
>>>> 	.read           =3D kcs_bmc_ipmi_read,
>>>>@@ -392,36 +392,58 @@ static const struct file_operations kcs_bmc_fops =
=3D {
>>>> 	.unlocked_ioctl =3D kcs_bmc_ipmi_ioctl,
>>>> };
>>>>
>>>>-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv=
, u32 channel);
>>>>-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv=
, u32 channel)
>>>>+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
>>>
>>>Errant declaration again?
>>
>>As previously explained.
>>
>
>This one seems like a slightly different category, because...
>
>>>
>>>>+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc)
>
>...it's immediately followed by the definition of the very same function
>that it just declared, so I can't see how its presence or absence could
>make any functional difference to anything.  (So perhaps I should have
>said "redundant" instead of "errant...again".)
>
>It's fairly trivial of course given that it's gone by the end of the
>series, but as long as there's going to be another iteration anyway it
>seems like we might as well tidy it up?
>

Oh, and otherwise:

Reviewed-by: Zev Weiss <zweiss@equinix.com>