Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write to avoid spinlock overhead

[Tomas Henzl <thenzl@redhat.com>]

On 09/06/2014 03:25 PM, Sumit.Saxena@avagotech.com wrote:
> Use writeq() for 64bit PCI write instead of writel() to avoid additional lock overhead.
>
> Signed-off-by: Sumit Saxena <sumit.saxena@avagotech.com>
> Signed-off-by: Kashyap Desai <kashyap.desai@avagotech.com>
> ---
>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index 57b47fe..c69c1ac 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -1065,6 +1065,13 @@ megasas_fire_cmd_fusion(struct megasas_instance *instance,
>  			u32 req_desc_hi,
>  			struct megasas_register_set __iomem *regs)

Hi Sumit,
the fn params are a bit confusing req_desc_lo is of type dma_addr_t
and req_desc_hi is u32, is it possible to unite it in the future?

>  {
> +#if defined(writeq) && defined(CONFIG_64BIT)

On a similar place mpt2sas(_base_writeq) uses only "#ifndef writeq"
if it's incorrect fix it there too or remove the CONFIG_64 here

> +	u64 req_data = 0;
> +
> +	req_data = req_desc_hi;
> +	req_data = ((req_data << 32) | (u32)req_desc_lo);

This seems to be critical path (you are removing an spinlock to avoid overhead),
so why do you have three consecutive assignments to the same variable?
(~(u64 req_data = r_hi << 32 | r_lo))

Cheers,
Tomas

> +	writeq(le64_to_cpu(req_data), &(regs)->inbound_low_queue_port);
> +#else
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&instance->hba_lock, flags);
> @@ -1072,6 +1079,7 @@ megasas_fire_cmd_fusion(struct megasas_instance *instance,
>  	writel(le32_to_cpu(req_desc_lo), &(regs)->inbound_low_queue_port);
>  	writel(le32_to_cpu(req_desc_hi), &(regs)->inbound_high_queue_port);
>  	spin_unlock_irqrestore(&instance->hba_lock, flags);
> +#endif
>  }
>  
>  /**