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

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

[Sergio Miguéns Iglesias]

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.

Signed-off-by: Sergio Miguéns Iglesias <sergio@lony.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

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 705 bytes --]

On 30.08.21 19:53, 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.
> 
> Signed-off-by: Sergio Miguéns Iglesias <sergio@lony.xyz>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Bjorn Helgaas]

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@lony.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
> 

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

[Sergio Miguéns Iglesias]

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

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

[Bjorn Helgaas]

On Mon, Aug 30, 2021 at 10:14:26PM +0200, Sergio Miguéns Iglesias wrote:
> 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.

If it's already applied, it doesn't matter for this case.  But in this
situation I would generally post a v3 incorporating the feedback.  To
be respectful of reviewers' time, try to avoid posting more than one
or two revisions per week.  As long as you tag reposts appropriately
with v2, v3, etc (as you did), there's no confusion.

It's nice if you include a note about what changed between v1 and v2
in the cover letter or below the "---" line as was done here:

  https://lore.kernel.org/linux-pci/8f9a13ac-8ab1-15ac-06cb-c131b488a36f@siemens.com/

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

[Sergio Miguéns Iglesias]

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@lony.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
> > 

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 697 bytes --]

On 30.08.21 19:53, 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.
> 
> Signed-off-by: Sergio Miguéns Iglesias <sergio@lony.xyz>

Pushed to xen/tip.git for-linus-5.15


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]