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

Tomas Henzl
2014-09-09T13:31:25Z

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
> }
>
> /**