| 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: <1516813514.7000.1235.camel@linux.intel.com>
Date: Wed, 24 Jan 2018 19:05:14 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Haiyue Wang <haiyue.wang@...ux.intel.com>, minyard@....org,
joel@....id.au, avifishman70@...il.com, openbmc@...ts.ozlabs.org,
openipmi-developer@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC
driver
On Thu, 2018-01-25 at 00:06 +0800, Haiyue Wang wrote:
> The KCS (Keyboard Controller Style) interface is used to perform in-
> band
> IPMI communication between a server host and its BMC (BaseBoard
> Management
> Controllers).
>
> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and
> AST2500)
> as a character device. Such SOCs are commonly used as BMCs and this
> driver
> implements the BMC side of the KCS interface.
> +config IPMI_KCS_BMC
> + tristate 'IPMI KCS BMC Interface'
> + help
> + Provides a device driver for the KCS (Keyboard Controller
> Style)
> + IPMI interface which meets the requirement of the BMC
> (Baseboard
> + Management Controllers) side for handling the IPMI request
> from
> + host system software.
Now time to split to two patches.
> +config ASPEED_KCS_IPMI_BMC
> + depends on ARCH_ASPEED || COMPILE_TEST
> + depends on IPMI_KCS_BMC
> + select REGMAP_MMIO
> + tristate "Aspeed KCS IPMI BMC driver"
> + help
> + Provides a driver for the KCS (Keyboard Controller Style)
> IPMI
> + interface found on Aspeed SOCs (AST2400 and AST2500).
> +
> + The driver implements the BMC side of the KCS contorller,
> it
> + provides the access of KCS IO space for BMC side.
> +obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
> obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
> +obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
> \ No newline at end of file
Do something with your text editor. The end of text file is a \n at the
end.
> +/* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */
> +#define KCS_STATUS_STATE(state) (state << 6)
> +#define KCS_STATUS_STATE_MASK KCS_STATUS_STATE(0x3)
GENMASK(8, 6)
> +
> +
Remove extra line in such cases
> +static inline u8 read_data(struct kcs_bmc *kcs_bmc)
> +{
> + return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
> +}
> +
> +static inline void write_data(struct kcs_bmc *kcs_bmc, u8 data)
> +{
> + kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
> +}
> +
> +static inline u8 read_status(struct kcs_bmc *kcs_bmc)
> +{
> + return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
> +}
> +
> +static inline void write_status(struct kcs_bmc *kcs_bmc, u8 data)
> +{
> + kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
> +}
> +
> +static void update_status_bits(struct kcs_bmc *kcs_bmc, u8 mask, u8
> val)
> +{
> + u8 tmp;
> +
> + tmp = read_status(kcs_bmc);
> +
> + tmp &= ~mask;
> + tmp |= val & mask;
> +
> + write_status(kcs_bmc, tmp);
> +}
Shouldn't be above some kind of regmap API?
> +int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> +{
> + unsigned long flags;
> + int ret = 0;
> + u8 status;
> +
> + spin_lock_irqsave(&kcs_bmc->lock, flags);
> +
> + status = read_status(kcs_bmc) & (KCS_STATUS_IBF |
> KCS_STATUS_CMD_DAT);
> +
> + switch (status) {
> + case KCS_STATUS_IBF | KCS_STATUS_CMD_DAT:
> + kcs_bmc_handle_command(kcs_bmc);
> + break;
> +
> + case KCS_STATUS_IBF:
> + kcs_bmc_handle_data(kcs_bmc);
> + break;
> +
> + default:
> + ret = -1;
Use proper errno.
> + break;
> + }
> +
> + spin_unlock_irqrestore(&kcs_bmc->lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(kcs_bmc_handle_event);
> +
> +static inline struct kcs_bmc *file_kcs_bmc(struct file *filp)
> +{
> + return container_of(filp->private_data, struct kcs_bmc,
> miscdev);
> +}
Such helper we call to_<smth>() where <smth> in your cases kcs_bmc
> +static ssize_t kcs_bmc_write(struct file *filp, const char *buf,
> + size_t count, loff_t *offset)
> +{
> + struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
> + ssize_t ret = count;
> +
> + if (count < 1 || count > KCS_MSG_BUFSIZ)
> + return -EINVAL;
Is the first part even possible?
> +}
> +struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv,
> u32 channel)
> +{
> + struct kcs_bmc *kcs_bmc;
> + int rc;
What compiler does think about this?
> +
> + kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc) + sizeof_priv,
> GFP_KERNEL);
> + if (!kcs_bmc)
> + return NULL;
> + dev_set_name(dev, "ipmi-kcs%u", channel);
> +
> + spin_lock_init(&kcs_bmc->lock);
> + kcs_bmc->channel = channel;
> +
> + init_waitqueue_head(&kcs_bmc->queue);
> + kcs_bmc->data_in = devm_kmalloc(dev, KCS_MSG_BUFSIZ,
> GFP_KERNEL);
> + kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ,
> GFP_KERNEL);
> + if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) {
> + dev_err(dev, "Failed to allocate data buffers\n");
> + return NULL;
> + }
Split checks per allocation.
> + kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
> + kcs_bmc->miscdev.name = dev_name(dev);
> + kcs_bmc->miscdev.fops = &kcs_bmc_fops;
> +
> + return kcs_bmc;
> +}
> +EXPORT_SYMBOL(kcs_bmc_alloc);
>
> +/* Different phases of the KCS BMC module */
> +enum kcs_phases {
> + /* BMC should not be expecting nor sending any data. */
> + KCS_PHASE_IDLE,
Perhaps kernel-doc?
> +};
> +/* IPMI 2.0 - 9.5, KCS Interface Registers */
> +struct kcs_ioreg {
> + u32 idr; /* Input Data Register */
> + u32 odr; /* Output Data Register */
> + u32 str; /* Status Register */
kernel-doc
> +};
> +
> +struct kcs_bmc {
> + spinlock_t lock;
> +
> + u32 channel;
> + int running;
> +
> + /* Setup by BMC KCS controller driver */
> + struct kcs_ioreg ioreg;
> + u8 (*io_inputb)(struct kcs_bmc *kcs_bmc, u32 reg);
> + void (*io_outputb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 b);
> +
> + enum kcs_phases phase;
> + enum kcs_errors error;
> +
> + wait_queue_head_t queue;
> + bool data_in_avail;
> + int data_in_idx;
> + u8 *data_in;
> +
> + int data_out_idx;
> + int data_out_len;
> + u8 *data_out;
> +
> + struct miscdevice miscdev;
> +
> + unsigned long long priv[];
unsigned long is enough.
> +};
> +
> +static inline void *kcs_bmc_priv(const struct kcs_bmc *kcs_bmc)
> +{
> + return kcs_bmc->priv;
> +}
> +
> +extern int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
> +extern struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int
> sizeof_priv,
> + u32 channel);
Drop extern.
> +#endif
Next one could be reviewed when you split this patch to two.
--
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy
Powered by blists - more mailing lists