| Message ID | 5756866F02000078000F269D@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/07/2016 02:31 AM, Jan Beulich wrote: > - drop unused function parameter of read_dev_bar() > - drop rom_init() (now identical to bar_init()) > - fold read_dev_bar() into its now single caller > - simplify determination of 64-bit memory resource > - use const and unsigned > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
On 07/06/16 07:31, Jan Beulich wrote: > - drop unused function parameter of read_dev_bar() > - drop rom_init() (now identical to bar_init()) > - fold read_dev_bar() into its now single caller > - simplify determination of 64-bit memory resource > - use const and unsigned Please split this in 5 separate patches for easier review. Especially as often anyone writing "simplify" means "accidentally break". David
>>> On 24.06.16 at 17:01, <david.vrabel@citrix.com> wrote: > On 07/06/16 07:31, Jan Beulich wrote: >> - drop unused function parameter of read_dev_bar() >> - drop rom_init() (now identical to bar_init()) >> - fold read_dev_bar() into its now single caller >> - simplify determination of 64-bit memory resource >> - use const and unsigned > > Please split this in 5 separate patches for easier review. > > Especially as often anyone writing "simplify" means "accidentally break". So this is directly opposite of what Boris had asked for - originally there were two patches, which I folded upon his request (and which he gave his R-b for already). May I ask the two of you to first settle on a consistent set of expectations to patches like this? Jan
On 27/06/16 08:24, Jan Beulich wrote: >>>> On 24.06.16 at 17:01, <david.vrabel@citrix.com> wrote: >> On 07/06/16 07:31, Jan Beulich wrote: >>> - drop unused function parameter of read_dev_bar() >>> - drop rom_init() (now identical to bar_init()) >>> - fold read_dev_bar() into its now single caller >>> - simplify determination of 64-bit memory resource >>> - use const and unsigned >> >> Please split this in 5 separate patches for easier review. >> >> Especially as often anyone writing "simplify" means "accidentally break". > > So this is directly opposite of what Boris had asked for - originally > there were two patches, which I folded upon his request (and > which he gave his R-b for already). May I ask the two of you to > first settle on a consistent set of expectations to patches like this? SubmittingPatches section 3 is clear on what is required. If your commit message is a list of bullet points it's a pretty big hint that none of the changes are related. David
--- 4.7-rc2-xen-pciback-BAR.orig/drivers/xen/xen-pciback/conf_space_header.c +++ 4.7-rc2-xen-pciback-BAR/drivers/xen/xen-pciback/conf_space_header.c @@ -209,58 +209,35 @@ static int bar_read(struct pci_dev *dev, return 0; } -static inline void read_dev_bar(struct pci_dev *dev, - struct pci_bar_info *bar_info, int offset, - u32 len_mask) +static void *bar_init(struct pci_dev *dev, int offset) { - int pos; - struct resource *res = dev->resource; + unsigned int pos; + const struct resource *res = dev->resource; + struct pci_bar_info *bar = kzalloc(sizeof(*bar), GFP_KERNEL); + + if (!bar) + return ERR_PTR(-ENOMEM); if (offset == PCI_ROM_ADDRESS || offset == PCI_ROM_ADDRESS1) pos = PCI_ROM_RESOURCE; else { pos = (offset - PCI_BASE_ADDRESS_0) / 4; - if (pos && ((res[pos - 1].flags & (PCI_BASE_ADDRESS_SPACE | - PCI_BASE_ADDRESS_MEM_TYPE_MASK)) == - (PCI_BASE_ADDRESS_SPACE_MEMORY | - PCI_BASE_ADDRESS_MEM_TYPE_64))) { - bar_info->val = res[pos - 1].start >> 32; - bar_info->len_val = -resource_size(&res[pos - 1]) >> 32; - return; + if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) { + bar->val = res[pos - 1].start >> 32; + bar->len_val = -resource_size(&res[pos - 1]) >> 32; + return bar; } } if (!res[pos].flags || (res[pos].flags & (IORESOURCE_DISABLED | IORESOURCE_UNSET | IORESOURCE_BUSY))) - return; - - bar_info->val = res[pos].start | - (res[pos].flags & PCI_REGION_FLAG_MASK); - bar_info->len_val = -resource_size(&res[pos]) | - (res[pos].flags & PCI_REGION_FLAG_MASK); -} + return bar; -static void *bar_init(struct pci_dev *dev, int offset) -{ - struct pci_bar_info *bar = kzalloc(sizeof(*bar), GFP_KERNEL); - - if (!bar) - return ERR_PTR(-ENOMEM); - - read_dev_bar(dev, bar, offset, ~0); - - return bar; -} - -static void *rom_init(struct pci_dev *dev, int offset) -{ - struct pci_bar_info *bar = kzalloc(sizeof(*bar), GFP_KERNEL); - - if (!bar) - return ERR_PTR(-ENOMEM); - - read_dev_bar(dev, bar, offset, ~PCI_ROM_ADDRESS_ENABLE); + bar->val = res[pos].start | + (res[pos].flags & PCI_REGION_FLAG_MASK); + bar->len_val = -resource_size(&res[pos]) | + (res[pos].flags & PCI_REGION_FLAG_MASK); return bar; } @@ -383,7 +360,7 @@ static const struct config_field header_ { \ .offset = reg_offset, \ .size = 4, \ - .init = rom_init, \ + .init = bar_init, \ .reset = bar_reset, \ .release = bar_release, \ .u.dw.read = bar_read, \
- drop unused function parameter of read_dev_bar() - drop rom_init() (now identical to bar_init()) - fold read_dev_bar() into its now single caller - simplify determination of 64-bit memory resource - use const and unsigned Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: fold in 3rd patch and drop read_dev_bar() (as requested by Boris) --- drivers/xen/xen-pciback/conf_space_header.c | 57 ++++++++-------------------- 1 file changed, 17 insertions(+), 40 deletions(-)