| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <997aa782-deb0-1d88-c144-8ea4537e55c2@os.amperecomputing.com>
Date: Mon, 20 Jun 2022 14:16:36 +0700
From: Quan Nguyen <quan@...amperecomputing.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: andrew@...id.au, benh@...nel.crashing.org,
brendanhiggins@...gle.com, devicetree@...r.kernel.org,
joel@....id.au, krzysztof.kozlowski+dt@...aro.org,
linux-arm-kernel@...ts.infradead.org,
linux-aspeed@...ts.ozlabs.org, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, minyard@....org,
openbmc@...ts.ozlabs.org, openipmi-developer@...ts.sourceforge.net,
patches@...erecomputing.com, phong@...amperecomputing.com,
robh+dt@...nel.org, thang@...amperecomputing.com, wsa@...nel.org
Subject: Re: [PATCH v8 1/3] ipmi: ssif_bmc: Add SSIF BMC driver
On 17/06/2022 03:47, Christophe JAILLET wrote:
> Le 15/06/2022 à 11:02, Quan Nguyen a écrit :
>> The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
>> in-band IPMI communication with their host in management (BMC) side.
>>
>> Thanks Dan for the copy_from_user() fix in the link below.
>>
>> Link:
>> https://lore.kernel.org/linux-arm-kernel/20220310114119.13736-4-quan-shex6MNQR2J/SfDzf78azzKzEDxYleXD@public.gmane.org/
>>
>> Signed-off-by: Quan Nguyen
>> <quan-shex6MNQR2J/SfDzf78azzKzEDxYleXD@...lic.gmane.org>
>> ---
>
> Hi,
>
> a few nitpick below
>
> [...]
>
>> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
>> new file mode 100644
>> index 000000000000..0bfd4b9bbaf1
>> --- /dev/null
>> +++ b/drivers/char/ipmi/ssif_bmc.c
>> @@ -0,0 +1,880 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * The driver for BMC side of SSIF interface
>> + *
>> + * Copyright (c) 2022, Ampere Computing LLC
>> + *
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/poll.h>
>> +#include <linux/sched.h>
>> +#include <linux/mutex.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/timer.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/ipmi_ssif_bmc.h>
>> +
>> +#define DEVICE_NAME "ipmi-ssif-host"
>> +
>> +#define GET_8BIT_ADDR(addr_7bit) (((addr_7bit) << 1) &
>> 0xff)
>> +
>> +/* A standard SMBus Transaction is limited to 32 data bytes */
>> +#define MAX_PAYLOAD_PER_TRANSACTION 32
>> +/* Transaction includes the address, the command, the length and the
>> PEC byte */
>> +#define MAX_TRANSACTION
>> (MAX_PAYLOAD_PER_TRANSACTION + 4)
>> +
>> +#define MAX_IPMI_DATA_PER_START_TRANSACTION 30
>> +#define MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION 31
>> +
>> +#define SSIF_IPMI_SINGLEPART_WRITE 0x2
>> +#define SSIF_IPMI_SINGLEPART_READ 0x3
>> +#define SSIF_IPMI_MULTIPART_WRITE_START 0x6
>> +#define SSIF_IPMI_MULTIPART_WRITE_MIDDLE 0x7
>> +#define SSIF_IPMI_MULTIPART_WRITE_END 0x8
>> +#define SSIF_IPMI_MULTIPART_READ_START 0x3
>> +#define SSIF_IPMI_MULTIPART_READ_MIDDLE 0x9
>> +
>> +/*
>> + * IPMI 2.0 Spec, section 12.7 SSIF Timing,
>> + * Request-to-Response Time is T6max(250ms) - T1max(20ms) - 3ms = 227ms
>> + * Recover ssif_bmc from busy state if it takes up to 500ms
>> + */
>> +#define RESPONSE_TIMEOUT 500 /* ms */
>> +
>> +struct ssif_part_buffer {
>> + u8 address;
>> + u8 smbus_cmd;
>> + u8 length;
>> + u8 payload[MAX_PAYLOAD_PER_TRANSACTION];
>> + u8 pec;
>> + u8 index;
>> +};
>> +
>> +/*
>> + * SSIF internal states:
>> + * SSIF_READY 0x00 : Ready state
>> + * SSIF_START 0x01 : Start smbus transaction
>> + * SSIF_SMBUS_CMD 0x02 : Received SMBus command
>> + * SSIF_REQ_RECVING 0x03 : Receiving request
>> + * SSIF_RES_SENDING 0x04 : Sending response
>> + * SSIF_BAD_SMBUS 0x05 : Bad SMbus transaction
>
> If these states are related to the enum just below,
> s/SSIF_BAD_SMBUS/SSIF_ABORTING/ + description update?
>
Thank you for this catch.
Will fix in new version.
>> + */
>> +enum ssif_state {
>> + SSIF_READY,
>> + SSIF_START,
>> + SSIF_SMBUS_CMD,
>> + SSIF_REQ_RECVING,
>> + SSIF_RES_SENDING,
>> + SSIF_ABORTING,
>> + SSIF_STATE_MAX
>> +};
>> +
>
> [...]
>
>> +static int ssif_bmc_probe(struct i2c_client *client, const struct
>> i2c_device_id *id)
>> +{
>> + struct ssif_bmc_ctx *ssif_bmc;
>> + int ret;
>> +
>> + ssif_bmc = devm_kzalloc(&client->dev, sizeof(*ssif_bmc),
>> GFP_KERNEL);
>> + if (!ssif_bmc)
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&ssif_bmc->lock);
>> +
>> + init_waitqueue_head(&ssif_bmc->wait_queue);
>> + ssif_bmc->request_available = false;
>> + ssif_bmc->response_in_progress = false;
>> + ssif_bmc->busy = false;
>> + ssif_bmc->response_timer_inited = false;
>> +
>> + /* Register misc device interface */
>> + ssif_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
>> + ssif_bmc->miscdev.name = DEVICE_NAME;
>> + ssif_bmc->miscdev.fops = &ssif_bmc_fops;
>> + ssif_bmc->miscdev.parent = &client->dev;
>> + ret = misc_register(&ssif_bmc->miscdev);
>> + if (ret)
>> + goto out;
>
> Could be "return ret;"
> (see below)
>
Will change to "return ret;" in next version
>> +
>> + ssif_bmc->client = client;
>> + ssif_bmc->client->flags |= I2C_CLIENT_SLAVE;
>> +
>> + /* Register I2C slave */
>> + i2c_set_clientdata(client, ssif_bmc);
>> + ret = i2c_slave_register(client, ssif_bmc_cb);
>> + if (ret) {
>> + misc_deregister(&ssif_bmc->miscdev);
>> + goto out;
>> + }
>> +
>> + return 0;
>> +out:
>> + devm_kfree(&client->dev, ssif_bmc);
>
> This looks useless to me. The whole error handling path could be
> removed, or updated to only have the "misc_deregister()" above.
>
Will rewrite as:
/* Register I2C slave */
i2c_set_clientdata(client, ssif_bmc);
ret = i2c_slave_register(client, ssif_bmc_cb);
if (ret)
misc_deregister(&ssif_bmc->miscdev);
return ret;
Thanks for the review.
- Quan
Powered by blists - more mailing lists