Re: [RFC V1 RESEND 5/6] PCI/MSI: Free MSI-X resources by group

[Megha Dey <megha.dey@intel.com>]

On Sun, 2019-08-11 at 09:18 +0200, Thomas Gleixner wrote:
> On Tue, 6 Aug 2019, Megha Dey wrote:
> 
> > 
> > On Sat, 2019-06-29 at 10:08 +0200, Thomas Gleixner wrote:
> > > 
> > > Megha,
> > > 
> > > On Fri, 21 Jun 2019, Megha Dey wrote:
> > > > 
> > > > 
> > > > +static int free_msi_irqs_grp(struct pci_dev *dev, int
> > > > group_id)
> > > > +{
> > > > 
> > > > +
> > > > +	for_each_pci_msi_entry(entry, dev) {
> > > > +		if (entry->group_id == group_id && entry->irq)
> > > > +			for (i = 0; i < entry->nvec_used; i++)
> > > > +				BUG_ON(irq_has_action(entry-
> > > > >irq +
> > > > i));
> > > BUG_ON is wrong here. This can and must be handled gracefully.
> > > 
> > Hmm, I reused this code from the 'free_msi_irqs' function. I am not
> > sure why it is wrong to use BUG_ON here but ok to use it there,
> > please
> > let me know.
> We are not adding BUG_ON() anymore except for situations where there
> is
> absolutely no way out. Just because there is still older code having
> BUG_ON() does not make it any better. Copying it surely is no
> justification.
> 
> If there is really no way out, then you need to explain it.
> 

Ok, got it. I will look into it closely to see if the BUG_ON is
absolutely required.

>  
> > 
> > > 
> > > > 
> > > > +static void pci_msix_shutdown_grp(struct pci_dev *dev, int
> > > > group_id)
> > > > +{
> > > > +	struct msi_desc *entry;
> > > > +	int grp_present = 0;
> > > > +
> > > > +	if (pci_dev_is_disconnected(dev)) {
> > > > +		dev->msix_enabled = 0;
> > > Huch? What's that? I can't figure out why this is needed and of
> > > course it
> > > completely lacks a comment explaining this. 
> > > 
> > Again, I have reused this code from the pci_msix_shutdown()
> > function.
> > So for the group case, this is not required?
> Copy and paste is not an argument, really. Can this happen here? If
> so,
> then please add a comment.
> 

Ok, will do.
> > 
> > > 
> > > > 
> > > > 
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	/* Return the device with MSI-X masked as initial
> > > > states
> > > > */
> > > > +	for_each_pci_msi_entry(entry, dev) {
> > > > +		if (entry->group_id == group_id) {
> > > > +			/* Keep cached states to be restored
> > > > */
> > > > +			__pci_msix_desc_mask_irq(entry, 1);
> > > > +			grp_present = 1;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (!grp_present) {
> > > > +		pci_err(dev, "Group to be disabled not
> > > > present\n");
> > > > +		return;
> > > So you print an error and silently return
> > > 
> > This is a void function, hence no error value can be returned. What
> > do
> > you think is the right thing to do if someone wants to delete a
> > group
> > which is not present?
> Well, you made it a void function. 

ok sure, got your point.
>  
> > 
> > > 
> > > > 
> > > > 
> > > > +	}
> > > > +}
> > > > +
> > > > +int pci_disable_msix_grp(struct pci_dev *dev, int group_id)
> > > > +{
> > > > +	int num_vecs;
> > > > +
> > > > +	if (!pci_msi_enable || !dev)
> > > > +		return -EINVAL;
> > > > +
> > > > +	pci_msix_shutdown_grp(dev, group_id);
> > > > +	num_vecs = free_msi_irqs_grp(dev, group_id);
> > > just to call in another function which has to do the same
> > > group_id
> > > lookup
> > > muck again.
> > Even with the new proposal, we are to have 2 sets of functions: one
> > to
> > delete all the msic_desc entries associated with the device, and
> > the
> > other to delete those only belonging a 'user specified' group. So
> > we do
> > need to pass a group_id to these functions right? Yes, internally
> > the
> > deletion would be straightforward with the new approach.
> That does not matter. If pci_msix_shutdown_grp() does not find a
> group, why
> proceeding instead of having a proper error return and telling the
> caller?
> 

Oh ok, I got it now, I will return a proper error code in the
pci_msi_shutdown_grp and do the free_msi_irqs_grp only if the group is
found.

> Thanks,
> 
> 	tglx