Messages in this thread Patch in this message | | | Subject | Re: [PATCH ipmi/kcs_bmc v2] ipmi: kcs_bmc: make the code be more clean | From | Corey Minyard <> | Date | Tue, 20 Feb 2018 07:29:18 -0600 |
| |
On 02/19/2018 09:55 AM, Haiyue Wang wrote: > --- When you use ---, it means everything following is not in the commit text, including your signature.
> v1 -> v2:
Do you want me to fold this into the previous patch? That's generally not how things work, a new patch is fine for this, with a list of things done like below.
One comment inline below...
> > Add 'SPDX-License-Identifier' style for header files modification. > --- > > 1. Add the missed key word '__user' for read / write. > 2. Remove the prefix 'file' of 'file_to_kcs_bmc', no need this > duplicated word as its parameter has 'struct file *filp'. > 3. Change the 'unsigned int' to '__poll_t' to meet the new 'poll' > definition. > 4. Correct the 'SPDX-License-Identifier' style for header files. > > Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com> > --- > drivers/char/ipmi/kcs_bmc.c | 32 +++++++++++++++++--------------- > drivers/char/ipmi/kcs_bmc.h | 6 ++++-- > drivers/char/ipmi/kcs_bmc_aspeed.c | 4 +++- > include/uapi/linux/ipmi_bmc.h | 6 ++++-- > 4 files changed, 28 insertions(+), 20 deletions(-) > > diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c > index 6476bfb..fbfc05e 100644 > --- a/drivers/char/ipmi/kcs_bmc.c > +++ b/drivers/char/ipmi/kcs_bmc.c > @@ -1,5 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > -// Copyright (c) 2015-2018, Intel Corporation. > +/* > + * Copyright (c) 2015-2018, Intel Corporation. > + */ > > #define pr_fmt(fmt) "kcs-bmc: " fmt > > @@ -242,14 +244,14 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc) > } > EXPORT_SYMBOL(kcs_bmc_handle_event); > > -static inline struct kcs_bmc *file_to_kcs_bmc(struct file *filp) > +static inline struct kcs_bmc *to_kcs_bmc(struct file *filp) > { > return container_of(filp->private_data, struct kcs_bmc, miscdev); > } > > static int kcs_bmc_open(struct inode *inode, struct file *filp) > { > - struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > + struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); > int ret = 0; > > spin_lock_irq(&kcs_bmc->lock); > @@ -262,25 +264,25 @@ static int kcs_bmc_open(struct inode *inode, struct file *filp) > return ret; > } > > -static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait) > +static __poll_t kcs_bmc_poll(struct file *filp, poll_table *wait) > { > - struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > - unsigned int mask = 0; > + struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); > + __poll_t mask = 0; > > poll_wait(filp, &kcs_bmc->queue, wait); > > spin_lock_irq(&kcs_bmc->lock); > if (kcs_bmc->data_in_avail) > - mask |= POLLIN; > + mask |= EPOLLIN;
I get this:
CC [M] drivers/char/ipmi/kcs_bmc.o ../drivers/char/ipmi/kcs_bmc.c: In function ‘kcs_bmc_poll’: ../drivers/char/ipmi/kcs_bmc.c:276:11: error: ‘EPOLLIN’ undeclared (first use in this function) mask |= EPOLLIN; ^ ../drivers/char/ipmi/kcs_bmc.c:276:11: note: each undeclared identifier is reported only once for each function it appears in
probably need to include linux/eventpoll.h
-corey
> spin_unlock_irq(&kcs_bmc->lock); > > return mask; > } > > -static ssize_t kcs_bmc_read(struct file *filp, char *buf, > - size_t count, loff_t *offset) > +static ssize_t kcs_bmc_read(struct file *filp, char __user *buf, > + size_t count, loff_t *ppos) > { > - struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > + struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); > bool data_avail; > size_t data_len; > ssize_t ret; > @@ -339,10 +341,10 @@ static ssize_t kcs_bmc_read(struct file *filp, char *buf, > return ret; > } > > -static ssize_t kcs_bmc_write(struct file *filp, const char *buf, > - size_t count, loff_t *offset) > +static ssize_t kcs_bmc_write(struct file *filp, const char __user *buf, > + size_t count, loff_t *ppos) > { > - struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > + struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); > ssize_t ret; > > /* a minimum response size '3' : netfn + cmd + ccode */ > @@ -378,7 +380,7 @@ static ssize_t kcs_bmc_write(struct file *filp, const char *buf, > static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > { > - struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > + struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); > long ret = 0; > > spin_lock_irq(&kcs_bmc->lock); > @@ -410,7 +412,7 @@ static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd, > > static int kcs_bmc_release(struct inode *inode, struct file *filp) > { > - struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > + struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); > > spin_lock_irq(&kcs_bmc->lock); > kcs_bmc->running = 0; > diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h > index c19501d..69d9a70 100644 > --- a/drivers/char/ipmi/kcs_bmc.h > +++ b/drivers/char/ipmi/kcs_bmc.h > @@ -1,5 +1,7 @@ > -// SPDX-License-Identifier: GPL-2.0 > -// Copyright (c) 2015-2018, Intel Corporation. > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2015-2018, Intel Corporation. > + */ > > #ifndef __KCS_BMC_H__ > #define __KCS_BMC_H__ > diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c > index 0c4d1a3..dba6075 100644 > --- a/drivers/char/ipmi/kcs_bmc_aspeed.c > +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c > @@ -1,5 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > -// Copyright (c) 2015-2018, Intel Corporation. > +/* > + * Copyright (c) 2015-2018, Intel Corporation. > + */ > > #define pr_fmt(fmt) "aspeed-kcs-bmc: " fmt > > diff --git a/include/uapi/linux/ipmi_bmc.h b/include/uapi/linux/ipmi_bmc.h > index 2f9f97e..d3efacd 100644 > --- a/include/uapi/linux/ipmi_bmc.h > +++ b/include/uapi/linux/ipmi_bmc.h > @@ -1,5 +1,7 @@ > -// SPDX-License-Identifier: GPL-2.0 > -// Copyright (c) 2015-2018, Intel Corporation. > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2015-2018, Intel Corporation. > + */ > > #ifndef _UAPI_LINUX_IPMI_BMC_H > #define _UAPI_LINUX_IPMI_BMC_H
| |