diff mbox series

[2/4] HID: intel-ish-hid: Optimize writing ipc message from queue

Message ID 20190212120523.27463-2-hong.liu@intel.com (mailing list archive)
State Mainlined
Commit b22f805bbe090d42e2eed86aa075687e47f924b7
Delegated to: Jiri Kosina
Headers show
Series [1/4] HID: intel-ish-hid: avoid binding wrong ishtp_cl_device | expand

Commit Message

Hong Liu Feb. 12, 2019, 12:05 p.m. UTC 
Currently we are using one additional static variable and a spinlock to
prevent contention of writing IPC messages to ISH hardware, which is
not necessary. Once ISH is ready to accept new data, we can push new
data to hardware. This pushing of new data is already protected by
wr_processing_spinlock for contention, which is enough. So use this
spinlock to check both readiness for accepting new data and once ready
allow writing of ipc message from queue to ISH hardware.

While here, cleaned up some space after return.

Signed-off-by: Hong Liu <hong.liu@intel.com>
Tested-by: Hongyan Song <hongyan.song@intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c         | 19 +++----------------
 drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h |  2 --
 2 files changed, 3 insertions(+), 18 deletions(-)

Comments

srinivas pandruvada Feb. 15, 2019, 1:47 p.m. UTC | #1 
On Tue, 2019-02-12 at 20:05 +0800, Hong Liu wrote:
> Currently we are using one additional static variable and a spinlock
> to
> prevent contention of writing IPC messages to ISH hardware, which is
> not necessary. Once ISH is ready to accept new data, we can push new
> data to hardware. This pushing of new data is already protected by
> wr_processing_spinlock for contention, which is enough. So use this
> spinlock to check both readiness for accepting new data and once
> ready
> allow writing of ipc message from queue to ISH hardware.
> 
> While here, cleaned up some space after return.
> 
> Signed-off-by: Hong Liu <hong.liu@intel.com>
> Tested-by: Hongyan Song <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/hid/intel-ish-hid/ipc/ipc.c         | 19 +++----------------
>  drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h |  2 --
>  2 files changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-
> ish-hid/ipc/ipc.c
> index 742191bb24c6..ff8eca11ff73 100644
> --- a/drivers/hid/intel-ish-hid/ipc/ipc.c
> +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
> @@ -256,33 +256,22 @@ static int write_ipc_from_queue(struct
> ishtp_device *dev)
>  	int	i;
>  	void	(*ipc_send_compl)(void *);
>  	void	*ipc_send_compl_prm;
> -	static int	out_ipc_locked;
> -	unsigned long	out_ipc_flags;
>  
>  	if (dev->dev_state == ISHTP_DEV_DISABLED)
> -		return	-EINVAL;
> +		return -EINVAL;
>  
> -	spin_lock_irqsave(&dev->out_ipc_spinlock, out_ipc_flags);
> -	if (out_ipc_locked) {
> -		spin_unlock_irqrestore(&dev->out_ipc_spinlock,
> out_ipc_flags);
> -		return -EBUSY;
> -	}
> -	out_ipc_locked = 1;
> +	spin_lock_irqsave(&dev->wr_processing_spinlock, flags);
>  	if (!ish_is_input_ready(dev)) {
> -		out_ipc_locked = 0;
> -		spin_unlock_irqrestore(&dev->out_ipc_spinlock,
> out_ipc_flags);
> +		spin_unlock_irqrestore(&dev->wr_processing_spinlock,
> flags);
>  		return -EBUSY;
>  	}
> -	spin_unlock_irqrestore(&dev->out_ipc_spinlock, out_ipc_flags);
>  
> -	spin_lock_irqsave(&dev->wr_processing_spinlock, flags);
>  	/*
>  	 * if tx send list is empty - return 0;
>  	 * may happen, as RX_COMPLETE handler doesn't check list
> emptiness.
>  	 */
>  	if (list_empty(&dev->wr_processing_list)) {
>  		spin_unlock_irqrestore(&dev->wr_processing_spinlock,
> flags);
> -		out_ipc_locked = 0;
>  		return	0;
>  	}
>  
> @@ -333,7 +322,6 @@ static int write_ipc_from_queue(struct
> ishtp_device *dev)
>  	dev->ipc_tx_bytes_cnt += IPC_HEADER_GET_LENGTH(doorbell_val);
>  
>  	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, doorbell_val);
> -	out_ipc_locked = 0;
>  
>  	ipc_send_compl = ipc_link->ipc_send_compl;
>  	ipc_send_compl_prm = ipc_link->ipc_send_compl_prm;
> @@ -914,7 +902,6 @@ struct ishtp_device *ish_dev_init(struct pci_dev
> *pdev)
>  	init_waitqueue_head(&dev->wait_hw_ready);
>  
>  	spin_lock_init(&dev->wr_processing_spinlock);
> -	spin_lock_init(&dev->out_ipc_spinlock);
>  
>  	/* Init IPC processing and free lists */
>  	INIT_LIST_HEAD(&dev->wr_processing_list);
> diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> index e7c6bfefaf9e..e54ce1ef27dd 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> +++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> @@ -211,8 +211,6 @@ struct ishtp_device {
>  	/* For both processing list  and free list */
>  	spinlock_t wr_processing_spinlock;
>  
> -	spinlock_t out_ipc_spinlock;
> -
>  	struct ishtp_fw_client *fw_clients; /*Note:memory has to be
> allocated*/
>  	DECLARE_BITMAP(fw_clients_map, ISHTP_CLIENTS_MAX);
>  	DECLARE_BITMAP(host_clients_map, ISHTP_CLIENTS_MAX);
diff mbox series

Patch

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 742191bb24c6..ff8eca11ff73 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -256,33 +256,22 @@  static int write_ipc_from_queue(struct ishtp_device *dev)
 	int	i;
 	void	(*ipc_send_compl)(void *);
 	void	*ipc_send_compl_prm;
-	static int	out_ipc_locked;
-	unsigned long	out_ipc_flags;
 
 	if (dev->dev_state == ISHTP_DEV_DISABLED)
-		return	-EINVAL;
+		return -EINVAL;
 
-	spin_lock_irqsave(&dev->out_ipc_spinlock, out_ipc_flags);
-	if (out_ipc_locked) {
-		spin_unlock_irqrestore(&dev->out_ipc_spinlock, out_ipc_flags);
-		return -EBUSY;
-	}
-	out_ipc_locked = 1;
+	spin_lock_irqsave(&dev->wr_processing_spinlock, flags);
 	if (!ish_is_input_ready(dev)) {
-		out_ipc_locked = 0;
-		spin_unlock_irqrestore(&dev->out_ipc_spinlock, out_ipc_flags);
+		spin_unlock_irqrestore(&dev->wr_processing_spinlock, flags);
 		return -EBUSY;
 	}
-	spin_unlock_irqrestore(&dev->out_ipc_spinlock, out_ipc_flags);
 
-	spin_lock_irqsave(&dev->wr_processing_spinlock, flags);
 	/*
 	 * if tx send list is empty - return 0;
 	 * may happen, as RX_COMPLETE handler doesn't check list emptiness.
 	 */
 	if (list_empty(&dev->wr_processing_list)) {
 		spin_unlock_irqrestore(&dev->wr_processing_spinlock, flags);
-		out_ipc_locked = 0;
 		return	0;
 	}
 
@@ -333,7 +322,6 @@  static int write_ipc_from_queue(struct ishtp_device *dev)
 	dev->ipc_tx_bytes_cnt += IPC_HEADER_GET_LENGTH(doorbell_val);
 
 	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, doorbell_val);
-	out_ipc_locked = 0;
 
 	ipc_send_compl = ipc_link->ipc_send_compl;
 	ipc_send_compl_prm = ipc_link->ipc_send_compl_prm;
@@ -914,7 +902,6 @@  struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
 	init_waitqueue_head(&dev->wait_hw_ready);
 
 	spin_lock_init(&dev->wr_processing_spinlock);
-	spin_lock_init(&dev->out_ipc_spinlock);
 
 	/* Init IPC processing and free lists */
 	INIT_LIST_HEAD(&dev->wr_processing_list);
diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
index e7c6bfefaf9e..e54ce1ef27dd 100644
--- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
+++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
@@ -211,8 +211,6 @@  struct ishtp_device {
 	/* For both processing list  and free list */
 	spinlock_t wr_processing_spinlock;
 
-	spinlock_t out_ipc_spinlock;
-
 	struct ishtp_fw_client *fw_clients; /*Note:memory has to be allocated*/
 	DECLARE_BITMAP(fw_clients_map, ISHTP_CLIENTS_MAX);
 	DECLARE_BITMAP(host_clients_map, ISHTP_CLIENTS_MAX);