Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 6 Dec 2016 00:37:08 +0000
Long Li <longli@microsoft.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Monday, December 5, 2016 8:53 AM
> > To: Long Li <longli@microsoft.com>
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; linux-
> > pci@vger.kernel.org
> > Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> > buffer
> > 
> > On Tue,  8 Nov 2016 14:04:38 -0800
> > Long Li <longli@exchange.microsoft.com> wrote:
> >   
> > > +	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> > > +
> > > +	params = &hbus->retarget_msi_interrupt_params;
> > > +	memset(params, 0, sizeof(*params));
> > > +	params->partition_id = HV_PARTITION_ID_SELF;
> > > +	params->source = 1; /* MSI(-X) */
> > > +	params->address = msi_desc->msg.address_lo;
> > > +	params->data = msi_desc->msg.data;
> > > +	params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> > >  			   (hbus->hdev->dev_instance.b[4] << 16) |
> > >  			   (hbus->hdev->dev_instance.b[7] << 8) |
> > >  			   (hbus->hdev->dev_instance.b[6] & 0xf8) |
> > >  			   PCI_FUNC(pdev->devfn);
> > > -	params.vector = cfg->vector;
> > > +	params->vector = cfg->vector;
> > >
> > >  	for_each_cpu_and(cpu, dest, cpu_online_mask)
> > > -		params.vp_mask |= (1ULL <<  
> > vmbus_cpu_number_to_vp_number(cpu));  
> > > +		params->vp_mask |= (1ULL <<  
> > vmbus_cpu_number_to_vp_number(cpu));  
> > > +
> > > +	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> > >
> > > -	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, &params, NULL);
> > > +	spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);  
> > 
> > It looks like the additional locking here is being overly paranoid.
> > The caller is already holding the irq descriptor lock. Look at fixup_irqs.  
> 
> You are right. On my test machine, there are two possible places calling hv_irq_unmask(): request _irq() and handle_edge_irq(). They both have desc->lock held when calling .irq_unmask on the chip. A review of the IRQ code shows that desc->lock is always held while calling chip->irq_unmask().
> 
> Since the lock doesn't do any harm and it is not on performance code path, we can remove the lock in the upcoming patches.

Why add it then remove it, your patch hasn't been accepted. Please revise it and remove it.
Don't add additional unnecessary code.