Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Reply-To: minyard@acm.org
Subject: Re: [PATCH ipmi/kcs_bmc v2] ipmi: kcs_bmc: make the code be more
 clean
To:     Haiyue Wang <haiyue.wang@linux.intel.com>,
        openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
References: <1519055731-18923-1-git-send-email-haiyue.wang@linux.intel.com>
From:   Corey Minyard <minyard@acm.org>
Message-ID: <9b5afddc-1c28-d5cf-d7ee-71a389592073@acm.org>
Date:   Tue, 20 Feb 2018 07:29:18 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <1519055731-18923-1-git-send-email-haiyue.wang@linux.intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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