Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1371854 > unrolled thread

Re: [PATCH v4 3/7] drivers:hv: Use new vmbus_mmio_free() from client drivers.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 3/7] drivers:hv: Use new vmbus_mmio_free() from client  drivers. Bjorn Helgaas <helgaas@kernel.org> - 2016-04-05 20:10 +0200
    RE: [PATCH v4 3/7] drivers:hv: Use new vmbus_mmio_free() from client  drivers. KY Srinivasan <kys@microsoft.com> - 2016-04-05 20:30 +0200
    RE: [PATCH v4 3/7] drivers:hv: Use new vmbus_mmio_free() from client  drivers. Jake Oshins <jakeo@microsoft.com> - 2016-04-05 22:30 +0200

#1371854 — Re: [PATCH v4 3/7] drivers:hv: Use new vmbus_mmio_free() from client drivers.

Hi Jake,

On Fri, Apr 01, 2016 at 05:47:43PM -0700, Jake Oshins wrote:
> This patch modifies all the callers of vmbus_mmio_allocate()
> to call vmbus_mmio_free() instead of release_mem_region().

This changelog merely restates the C code.  Presumably there's some
important difference between release_mem_region() and
vmbus_mmio_free(), and we need a hint about what that is.

Oh, I see, there actually is no difference *yet*, but it's coming.
I'd combine this with patch 2.  Then the patch is obviously correct
all by itself, and the changelog for patch 2 makes clear what's
happening.

In changelogs, don't bother with "this patch does" or "this function
is introduced."  The context is obvious because the changelog is part
of the commit.  Write imperative sentences, e.g., "Call
vmbus_mmio_free() instead of release_mem_region()."

> Signed-off-by: Jake Oshins <jakeo@microsoft.com>

I think this is the only change that touches PCI, so I assume this
series will be merged by somebody else.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/host/pci-hyperv.c   | 14 +++++++-------
>  drivers/video/fbdev/hyperv_fb.c |  4 ++--
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index ed651ba..f2559b6 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1795,14 +1795,14 @@ static void hv_pci_free_bridge_windows(struct hv_pcibus_device *hbus)
>  
>  	if (hbus->low_mmio_space && hbus->low_mmio_res) {
>  		hbus->low_mmio_res->flags |= IORESOURCE_BUSY;
> -		release_mem_region(hbus->low_mmio_res->start,
> -				   resource_size(hbus->low_mmio_res));
> +		vmbus_free_mmio(hbus->low_mmio_res->start,
> +				resource_size(hbus->low_mmio_res));
>  	}
>  
>  	if (hbus->high_mmio_space && hbus->high_mmio_res) {
>  		hbus->high_mmio_res->flags |= IORESOURCE_BUSY;
> -		release_mem_region(hbus->high_mmio_res->start,
> -				   resource_size(hbus->high_mmio_res));
> +		vmbus_free_mmio(hbus->high_mmio_res->start,
> +				resource_size(hbus->high_mmio_res));
>  	}
>  }
>  
> @@ -1880,8 +1880,8 @@ static int hv_pci_allocate_bridge_windows(struct hv_pcibus_device *hbus)
>  
>  release_low_mmio:
>  	if (hbus->low_mmio_res) {
> -		release_mem_region(hbus->low_mmio_res->start,
> -				   resource_size(hbus->low_mmio_res));
> +		vmbus_free_mmio(hbus->low_mmio_res->start,
> +				resource_size(hbus->low_mmio_res));
>  	}
>  
>  	return ret;
> @@ -1924,7 +1924,7 @@ static int hv_allocate_config_window(struct hv_pcibus_device *hbus)
>  
>  static void hv_free_config_window(struct hv_pcibus_device *hbus)
>  {
> -	release_mem_region(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
> +	vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
>  }
>  
>  /**
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index e2451bd..2fd49b2 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -743,7 +743,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>  err3:
>  	iounmap(fb_virt);
>  err2:
> -	release_mem_region(par->mem->start, screen_fb_size);
> +	vmbus_free_mmio(par->mem->start, screen_fb_size);
>  	par->mem = NULL;
>  err1:
>  	if (!gen2vm)
> @@ -758,7 +758,7 @@ static void hvfb_putmem(struct fb_info *info)
>  	struct hvfb_par *par = info->par;
>  
>  	iounmap(info->screen_base);
> -	release_mem_region(par->mem->start, screen_fb_size);
> +	vmbus_free_mmio(par->mem->start, screen_fb_size);
>  	par->mem = NULL;
>  }
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [next] | [standalone]

#1371876

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Tuesday, April 5, 2016 11:00 AM
> To: Jake Oshins <jakeo@microsoft.com>
> Cc: linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; KY Srinivasan
> <kys@microsoft.com>; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> vkuznets@redhat.com; Haiyang Zhang <haiyangz@microsoft.com>; Hadden
> Hoppert <haddenh@microsoft.com>
> Subject: Re: [PATCH v4 3/7] drivers:hv: Use new vmbus_mmio_free() from
> client drivers.
> 
> Hi Jake,
> 
> On Fri, Apr 01, 2016 at 05:47:43PM -0700, Jake Oshins wrote:
> > This patch modifies all the callers of vmbus_mmio_allocate()
> > to call vmbus_mmio_free() instead of release_mem_region().
> 
> This changelog merely restates the C code.  Presumably there's some
> important difference between release_mem_region() and
> vmbus_mmio_free(), and we need a hint about what that is.
> 
> Oh, I see, there actually is no difference *yet*, but it's coming.
> I'd combine this with patch 2.  Then the patch is obviously correct
> all by itself, and the changelog for patch 2 makes clear what's
> happening.
> 
> In changelogs, don't bother with "this patch does" or "this function
> is introduced."  The context is obvious because the changelog is part
> of the commit.  Write imperative sentences, e.g., "Call
> vmbus_mmio_free() instead of release_mem_region()."
> 
> > Signed-off-by: Jake Oshins <jakeo@microsoft.com>
> 
> I think this is the only change that touches PCI, so I assume this
> series will be merged by somebody else.
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I am hoping this will go through Greg's tree.

K. Y
> 
> > ---
> >  drivers/pci/host/pci-hyperv.c   | 14 +++++++-------
> >  drivers/video/fbdev/hyperv_fb.c |  4 ++--
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> > index ed651ba..f2559b6 100644
> > --- a/drivers/pci/host/pci-hyperv.c
> > +++ b/drivers/pci/host/pci-hyperv.c
> > @@ -1795,14 +1795,14 @@ static void hv_pci_free_bridge_windows(struct
> hv_pcibus_device *hbus)
> >
> >  	if (hbus->low_mmio_space && hbus->low_mmio_res) {
> >  		hbus->low_mmio_res->flags |= IORESOURCE_BUSY;
> > -		release_mem_region(hbus->low_mmio_res->start,
> > -				   resource_size(hbus->low_mmio_res));
> > +		vmbus_free_mmio(hbus->low_mmio_res->start,
> > +				resource_size(hbus->low_mmio_res));
> >  	}
> >
> >  	if (hbus->high_mmio_space && hbus->high_mmio_res) {
> >  		hbus->high_mmio_res->flags |= IORESOURCE_BUSY;
> > -		release_mem_region(hbus->high_mmio_res->start,
> > -				   resource_size(hbus->high_mmio_res));
> > +		vmbus_free_mmio(hbus->high_mmio_res->start,
> > +				resource_size(hbus->high_mmio_res));
> >  	}
> >  }
> >
> > @@ -1880,8 +1880,8 @@ static int hv_pci_allocate_bridge_windows(struct
> hv_pcibus_device *hbus)
> >
> >  release_low_mmio:
> >  	if (hbus->low_mmio_res) {
> > -		release_mem_region(hbus->low_mmio_res->start,
> > -				   resource_size(hbus->low_mmio_res));
> > +		vmbus_free_mmio(hbus->low_mmio_res->start,
> > +				resource_size(hbus->low_mmio_res));
> >  	}
> >
> >  	return ret;
> > @@ -1924,7 +1924,7 @@ static int hv_allocate_config_window(struct
> hv_pcibus_device *hbus)
> >
> >  static void hv_free_config_window(struct hv_pcibus_device *hbus)
> >  {
> > -	release_mem_region(hbus->mem_config->start,
> PCI_CONFIG_MMIO_LENGTH);
> > +	vmbus_free_mmio(hbus->mem_config->start,
> PCI_CONFIG_MMIO_LENGTH);
> >  }
> >
> >  /**
> > diff --git a/drivers/video/fbdev/hyperv_fb.c
> b/drivers/video/fbdev/hyperv_fb.c
> > index e2451bd..2fd49b2 100644
> > --- a/drivers/video/fbdev/hyperv_fb.c
> > +++ b/drivers/video/fbdev/hyperv_fb.c
> > @@ -743,7 +743,7 @@ static int hvfb_getmem(struct hv_device *hdev,
> struct fb_info *info)
> >  err3:
> >  	iounmap(fb_virt);
> >  err2:
> > -	release_mem_region(par->mem->start, screen_fb_size);
> > +	vmbus_free_mmio(par->mem->start, screen_fb_size);
> >  	par->mem = NULL;
> >  err1:
> >  	if (!gen2vm)
> > @@ -758,7 +758,7 @@ static void hvfb_putmem(struct fb_info *info)
> >  	struct hvfb_par *par = info->par;
> >
> >  	iounmap(info->screen_base);
> > -	release_mem_region(par->mem->start, screen_fb_size);
> > +	vmbus_free_mmio(par->mem->start, screen_fb_size);
> >  	par->mem = NULL;
> >  }
> >
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fvger.ker
> nel.org%2fmajordomo-
> info.html&data=01%7c01%7ckys%40microsoft.com%7cf391db0d7312429368e
> 908d35d7c17ed%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=bH6jnta
> OKCSn%2fZw%2bCN1P9wV9%2bRk0om8hw4evDezjR0Q%3d

[toc] | [prev] | [next] | [standalone]

#1371918

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Tuesday, April 5, 2016 11:00 AM
> To: Jake Oshins <jakeo@microsoft.com>
> Cc: linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; KY Srinivasan
> <kys@microsoft.com>; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> vkuznets@redhat.com; Haiyang Zhang <haiyangz@microsoft.com>; Hadden
> Hoppert <haddenh@microsoft.com>
> Subject: Re: [PATCH v4 3/7] drivers:hv: Use new vmbus_mmio_free() from
> client drivers.
> 
> Hi Jake,
> 
> On Fri, Apr 01, 2016 at 05:47:43PM -0700, Jake Oshins wrote:
> > This patch modifies all the callers of vmbus_mmio_allocate()
> > to call vmbus_mmio_free() instead of release_mem_region().
> 
> This changelog merely restates the C code.  Presumably there's some
> important difference between release_mem_region() and
> vmbus_mmio_free(), and we need a hint about what that is.
> 
> Oh, I see, there actually is no difference *yet*, but it's coming.
> I'd combine this with patch 2.  Then the patch is obviously correct
> all by itself, and the changelog for patch 2 makes clear what's
> happening.
> 
> In changelogs, don't bother with "this patch does" or "this function
> is introduced."  The context is obvious because the changelog is part
> of the commit.  Write imperative sentences, e.g., "Call
> vmbus_mmio_free() instead of release_mem_region()."
> 

Will do.  Thanks.

-- Jake Oshins

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web