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 > >
