Re: [PATCH v2] xen/pcifront: Removed unnecessary __ref annotation

From: Sergio Miguéns Iglesias
Date: Mon Aug 30 2021 - 14:14:37 EST


Thanks again for you answers!
I am lerning a lot from your replys and I really appreciate it. Should I
make a v3 patch and split that one into 2 different patches or would it
be confusing?

I don't want to take more of your time with poor patches so I don't know
if I should resend this one.

Thanks again,
Sergio M. Iglesias.

On 21/08/30 11:29, Bjorn Helgaas wrote:
> On Mon, Aug 30, 2021 at 07:53:05PM +0200, Sergio Miguéns Iglesias wrote:
> > An unnecessary "__ref" annotation was removed from the
> > "drivers/pci/xen_pcifront.c" file. The function where the annotation
> > was used was "pcifront_backend_changed()", which does not call any
> > functions annotated as "__*init" nor "__*exit". This makes "__ref"
> > unnecessary since this annotation is used to make the compiler ignore
> > section miss-matches when they are not happening here in the first
> > place.
> >
> > In addition to the aforementioned change, some code style issues were
> > fixed in the same file.
>
> One of the Xen folks may apply this, and they may not be as nit-picky
> as I am :)
>
> If I were to apply this, I would suggest:
>
> - Write subject line and commit message in imperative mood. This is
> a really good guide to this and other commit message this:
> https://chris.beams.io/posts/git-commit/
>
> For example, in the subject, say "Remove" (not "Removed"). Same
> in the body. In the body, I would mention the function but not
> the filename since that's obvious from the diff.
>
> - Split the __ref change into a separate patch from the style
> changes. The __ref removal should come first and be the absolute
> minimal patch. That makes it much easier to review, backport, and
> revert if necessary. And, if the maintainer isn't wild about
> style patches, it's trivial to just ignore that patch.
>
> Commit logs that say "Also, ..." or "In addition, ..." are always
> red flags to me because they usually indicate the patch could be
> split into two or more simpler patches.
>
> - When reviewing changes like this, I assume __ref was added in the
> first place for some good reason, so I want to know why, and I
> want to know when that reason changed. So I would look for the
> commit that *introduced* __ref and for the commit that removed the
> need for it. It would save me time if the log said something
> like:
>
> 956a9202cd12 ("xen-pcifront: Xen PCI frontend driver.") added
> __initrefok because pcifront_backend_changed() called
> pcifront_try_connect() and pcifront_attach_devices(), which were
> both __devinit.
>
> The __devinit annotations were removed by 15856ad50bf5 ("PCI:
> Remove __dev* markings"), which made __initrefok unnecessary.
>
> Finally, bd721ea73e1f ("treewide: replace obsolete _refok by
> __ref") replaced __initrefok with __ref.
>
> That might be too much for a commit log, but it shows that you've
> done your homework and makes it easier to review (and helps people
> make similar fixes elsewhere). If it *is* too much, it's trivial
> for a maintainer to cut it out.
>
> More notes about my idiosyncracies:
> https://lore.kernel.org/r/20171026223701.GA25649@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>
> > Signed-off-by: Sergio Miguéns Iglesias <sergio@xxxxxxxx>
> > ---
> > drivers/pci/xen-pcifront.c | 30 +++++++++++++++++++-----------
> > 1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> > index b7a8f3a1921f..427041c1e408 100644
> > --- a/drivers/pci/xen-pcifront.c
> > +++ b/drivers/pci/xen-pcifront.c
> > @@ -115,7 +115,7 @@ static int do_pci_op(struct pcifront_device *pdev, struct xen_pci_op *op)
> > struct xen_pci_op *active_op = &pdev->sh_info->op;
> > unsigned long irq_flags;
> > evtchn_port_t port = pdev->evtchn;
> > - unsigned irq = pdev->irq;
> > + unsigned int irq = pdev->irq;
> > s64 ns, ns_timeout;
> >
> > spin_lock_irqsave(&pdev->sh_info_lock, irq_flags);
> > @@ -153,10 +153,10 @@ static int do_pci_op(struct pcifront_device *pdev, struct xen_pci_op *op)
> > }
> >
> > /*
> > - * We might lose backend service request since we
> > - * reuse same evtchn with pci_conf backend response. So re-schedule
> > - * aer pcifront service.
> > - */
> > + * We might lose backend service request since we
> > + * reuse same evtchn with pci_conf backend response. So re-schedule
> > + * aer pcifront service.
> > + */
> > if (test_bit(_XEN_PCIB_active,
> > (unsigned long *)&pdev->sh_info->flags)) {
> > dev_err(&pdev->xdev->dev,
> > @@ -414,7 +414,8 @@ static int pcifront_scan_bus(struct pcifront_device *pdev,
> > struct pci_dev *d;
> > unsigned int devfn;
> >
> > - /* Scan the bus for functions and add.
> > + /*
> > + * Scan the bus for functions and add.
> > * We omit handling of PCI bridge attachment because pciback prevents
> > * bridges from being exported.
> > */
> > @@ -492,8 +493,10 @@ static int pcifront_scan_root(struct pcifront_device *pdev,
> >
> > list_add(&bus_entry->list, &pdev->root_buses);
> >
> > - /* pci_scan_root_bus skips devices which do not have a
> > - * devfn==0. The pcifront_scan_bus enumerates all devfn. */
> > + /*
> > + * pci_scan_root_bus skips devices which do not have a
> > + * devfn==0. The pcifront_scan_bus enumerates all devfn.
> > + */
> > err = pcifront_scan_bus(pdev, domain, bus, b);
> >
> > /* Claim resources before going "live" with our devices */
> > @@ -651,8 +654,10 @@ static void pcifront_do_aer(struct work_struct *data)
> > pci_channel_state_t state =
> > (pci_channel_state_t)pdev->sh_info->aer_op.err;
> >
> > - /*If a pci_conf op is in progress,
> > - we have to wait until it is done before service aer op*/
> > + /*
> > + * If a pci_conf op is in progress, we have to wait until it is done
> > + * before service aer op
> > + */
> > dev_dbg(&pdev->xdev->dev,
> > "pcifront service aer bus %x devfn %x\n",
> > pdev->sh_info->aer_op.bus, pdev->sh_info->aer_op.devfn);
> > @@ -676,6 +681,7 @@ static void pcifront_do_aer(struct work_struct *data)
> > static irqreturn_t pcifront_handler_aer(int irq, void *dev)
> > {
> > struct pcifront_device *pdev = dev;
> > +
> > schedule_pcifront_aer_op(pdev);
> > return IRQ_HANDLED;
> > }
> > @@ -1027,6 +1033,7 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> > /* Find devices being detached and remove them. */
> > for (i = 0; i < num_devs; i++) {
> > int l, state;
> > +
> > l = snprintf(str, sizeof(str), "state-%d", i);
> > if (unlikely(l >= (sizeof(str) - 1))) {
> > err = -ENOMEM;
> > @@ -1078,7 +1085,7 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> > return err;
> > }
> >
> > -static void __ref pcifront_backend_changed(struct xenbus_device *xdev,
> > +static void pcifront_backend_changed(struct xenbus_device *xdev,
> > enum xenbus_state be_state)
> > {
> > struct pcifront_device *pdev = dev_get_drvdata(&xdev->dev);
> > @@ -1137,6 +1144,7 @@ static int pcifront_xenbus_probe(struct xenbus_device *xdev,
> > static int pcifront_xenbus_remove(struct xenbus_device *xdev)
> > {
> > struct pcifront_device *pdev = dev_get_drvdata(&xdev->dev);
> > +
> > if (pdev)
> > free_pdev(pdev);
> >
> > --
> > 2.33.0
> >