Messages in this thread | | | Date | Thu, 16 Jun 2022 22:47:39 +0200 | Subject | Re: [PATCH v8 1/3] ipmi: ssif_bmc: Add SSIF BMC driver | From | Christophe JAILLET <> |
| |
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@public.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?
> + */ > +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)
> + > + 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.
CJ
> + return ret; > +}
| |