Message-ID: <20210830202133.q6j2he5kijf2tgpy@archlap>
Date:   Mon, 30 Aug 2021 22:21:33 +0200
From:   Sergio Miguéns Iglesias <lonyelon@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     konrad.wilk@...cle.com, boris.ostrovsky@...cle.com,
        jgross@...e.com, sstabellini@...nel.org, bhelgaas@...gle.com,
        xen-devel@...ts.xenproject.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sergio Miguéns Iglesias <sergio@...y.xyz>
Subject: Re: [PATCH v2] xen/pcifront: Removed unnecessary __ref annotation

Never mind, it got accepted anyways, but I will 100% fix my commit
messages for my future patches.

I really appreciate your suggestions and the time you have put into
writing them. I will improve in my next commits :)

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@bhelgaas-glaptop.roam.corp.google.com
> 
> > Signed-off-by: Sergio Miguéns Iglesias <sergio@...y.xyz>
> > ---
> >  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
> > 

Powered by blists - more mailing lists