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>,
        "minyard@acm.org" <minyard@acm.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "ryan_chen@aspeedtech.com" <ryan_chen@aspeedtech.com>,
        "tmaimon77@gmail.com" <tmaimon77@gmail.com>,
        "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
        "avifishman70@gmail.com" <avifishman70@gmail.com>,
        "venture@google.com" <venture@google.com>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "tali.perry1@gmail.com" <tali.perry1@gmail.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "lee.jones@linaro.org" <lee.jones@linaro.org>,
        "chiawei_wang@aspeedtech.com" <chiawei_wang@aspeedtech.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "benjaminfair@google.com" <benjaminfair@google.com>
Subject: Re: [PATCH v2 15/21] ipmi: kcs_bmc: Don't enforce single-open policy
 in the kernel
Thread-Topic: [PATCH v2 15/21] ipmi: kcs_bmc: Don't enforce single-open policy
 in the kernel
Thread-Index: AQHXLP4yV1bPwWXIUE6uiUHJ1sIkdw==
Date:   Fri, 9 Apr 2021 05:07:11 +0000
Message-ID: <YG/g/poZLwO34QH7@packtop>
References: <20210319062752.145730-1-andrew@aj.id.au>
 <20210319062752.145730-15-andrew@aj.id.au>
In-Reply-To: <20210319062752.145730-15-andrew@aj.id.au>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4AFF152F614E864091DBF9A7AB886B87@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 12788c7b-e50f-4a19-e12f-08d8fb155501
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Apr 2021 05:07:11.1506
 (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: /fZl6HVF5n+m30geKhwEbIGHPif8SYJ+knyeCNrqM1sE8xh0bP0CV18msaU56zR8tlxTJ/H02jIn+T5qS8Y2+Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR04MB1034
X-Proofpoint-GUID: xoTdrgzNs9ersZl9TD_dsMaivlxdYq4U
X-Proofpoint-ORIG-GUID: xoTdrgzNs9ersZl9TD_dsMaivlxdYq4U
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761
 definitions=2021-04-09_03:2021-04-08,2021-04-09 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501
 lowpriorityscore=0 phishscore=0 impostorscore=0 suspectscore=0
 malwarescore=0 bulkscore=0 adultscore=0 mlxlogscore=999 mlxscore=0
 spamscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104060000 definitions=main-2104090036
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Mar 19, 2021 at 01:27:46AM CDT, Andrew Jeffery wrote:
>Soon it will be possible for one KCS device to have multiple associated
>chardevs exposed to userspace (for IPMI and raw-style access). However,
>don't prevent userspace from:
>
>1. Opening more than one chardev at a time, or
>2. Opening the same chardev more than once.
>
>System behaviour is undefined for both classes of multiple access, so
>userspace must manage itself accordingly.
>
>The implementation delivers IBF and OBF events to the first chardev
>client to associate with the KCS device. An open on a related chardev
>cannot associate its client with the KCS device and so will not
>receive notification of events. However, any fd on any chardev may race
>their accesses to the data and status registers.
>
>Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>---
> drivers/char/ipmi/kcs_bmc.c         | 34 ++++++++++-------------------
> drivers/char/ipmi/kcs_bmc_aspeed.c  |  3 +--
> drivers/char/ipmi/kcs_bmc_npcm7xx.c |  3 +--
> 3 files changed, 14 insertions(+), 26 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>index 05bbb72418b2..2fafa9541934 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -55,24 +55,12 @@ EXPORT_SYMBOL(kcs_bmc_update_status);
> int kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc)
> {
> 	struct kcs_bmc_client *client;
>-	int rc;
>+	int rc =3D KCS_BMC_EVENT_NONE;
>
> 	spin_lock(&kcs_bmc->lock);
> 	client =3D kcs_bmc->client;
>-	if (client) {
>+	if (!WARN_ON_ONCE(!client))
> 		rc =3D client->ops->event(client);

The double-negation split by a macro seems a bit confusing to me
readability-wise; could we simplify to something like

	if (client)
		rc =3D client->ops->event(client);
	else
		WARN_ONCE();

?

>-	} else {
>-		u8 status;
>-
>-		status =3D kcs_bmc_read_status(kcs_bmc);
>-		if (status & KCS_BMC_STR_IBF) {
>-			/* Ack the event by reading the data */
>-			kcs_bmc_read_data(kcs_bmc);
>-			rc =3D KCS_BMC_EVENT_HANDLED;
>-		} else {
>-			rc =3D KCS_BMC_EVENT_NONE;
>-		}
>-	}
> 	spin_unlock(&kcs_bmc->lock);
>
> 	return rc;
>@@ -81,26 +69,28 @@ EXPORT_SYMBOL(kcs_bmc_handle_event);
>
> int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_=
client *client)
> {
>-	int rc;
>-
> 	spin_lock_irq(&kcs_bmc->lock);
>-	if (kcs_bmc->client) {
>-		rc =3D -EBUSY;
>-	} else {
>+	if (!kcs_bmc->client) {
>+		u8 mask =3D KCS_BMC_EVENT_TYPE_IBF;
>+
> 		kcs_bmc->client =3D client;
>-		rc =3D 0;
>+		kcs_bmc_update_event_mask(kcs_bmc, mask, mask);
> 	}
> 	spin_unlock_irq(&kcs_bmc->lock);
>
>-	return rc;
>+	return 0;

Since this function appears to be infallible now, should it just return
void?  (Might be more churn than it's worth...shrug.)

> }
> EXPORT_SYMBOL(kcs_bmc_enable_device);
>
> void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bm=
c_client *client)
> {
> 	spin_lock_irq(&kcs_bmc->lock);
>-	if (client =3D=3D kcs_bmc->client)
>+	if (client =3D=3D kcs_bmc->client) {
>+		u8 mask =3D KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE;
>+
>+		kcs_bmc_update_event_mask(kcs_bmc, mask, 0);
> 		kcs_bmc->client =3D NULL;
>+	}
> 	spin_unlock_irq(&kcs_bmc->lock);
> }
> EXPORT_SYMBOL(kcs_bmc_disable_device);
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bm=
c_aspeed.c
>index 5f26471c038c..271845eb2e26 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -419,8 +419,7 @@ static int aspeed_kcs_probe(struct platform_device *pd=
ev)
>
> 	platform_set_drvdata(pdev, priv);
>
>-	aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EV=
ENT_TYPE_OBE),
>-				   KCS_BMC_EVENT_TYPE_IBF);
>+	aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EV=
ENT_TYPE_OBE), 0);
> 	aspeed_kcs_enable_channel(kcs_bmc, true);
>
> 	rc =3D kcs_bmc_add_device(&priv->kcs_bmc);
>diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_b=
mc_npcm7xx.c
>index c2032728a03d..fdf35cad2eba 100644
>--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>@@ -207,8 +207,7 @@ static int npcm7xx_kcs_probe(struct platform_device *p=
dev)
> 	if (rc)
> 		return rc;
>
>-	npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_E=
VENT_TYPE_OBE),
>-				    KCS_BMC_EVENT_TYPE_IBF);
>+	npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_E=
VENT_TYPE_OBE), 0);
> 	npcm7xx_kcs_enable_channel(kcs_bmc, true);
>
> 	pr_info("channel=3D%u idr=3D0x%x odr=3D0x%x str=3D0x%x\n",
>--=20
>2.27.0
>=