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

Groups > linux.kernel > #1719732

Re: [PATCH v7 04/12] powerpc/vas: Define helpers to access MMIO regions

Show all headers | View raw

Hi Suka,

Comments inline.

Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> writes:
> diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> index 6156fbe..a3a705a 100644
> --- a/arch/powerpc/platforms/powernv/vas-window.c
> +++ b/arch/powerpc/platforms/powernv/vas-window.c
> @@ -9,9 +9,182 @@
>  
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
>  
>  #include "vas.h"
>  
> +/*
> + * Compute the paste address region for the window @window using the
> + * ->paste_base_addr and ->paste_win_id_shift we got from device tree.
> + */
> +void compute_paste_address(struct vas_window *window, uint64_t *addr, int *len)
> +{
> +	uint64_t base, shift;

Please use the kernel types, so u64 here.

> +	int winid;
> +
> +	base = window->vinst->paste_base_addr;
> +	shift = window->vinst->paste_win_id_shift;
> +	winid = window->winid;
> +
> +	*addr  = base + (winid << shift);
> +	if (len)
> +		*len = PAGE_SIZE;

Having multiple output parameters makes for a pretty awkward API. Is it
really necesssary given len is a constant PAGE_SIZE anyway.

If you didn't return len, then you could just make the function return
the addr, and you wouldn't need any output parameters.

One of the callers that passes len is unmap_paste_region(), but that
is a bit odd. It would be more natural I think if once a window is
mapped it knows its size. Or if the mapping will always just be one page
then we can just know that.

> +
> +	pr_debug("Txwin #%d: Paste addr 0x%llx\n", winid, *addr);
> +}
> +
> +static inline void get_hvwc_mmio_bar(struct vas_window *window,
> +			uint64_t *start, int *len)
> +{
> +	uint64_t pbaddr;
> +
> +	pbaddr = window->vinst->hvwc_bar_start;
> +	*start = pbaddr + window->winid * VAS_HVWC_SIZE;
> +	*len = VAS_HVWC_SIZE;

This is:

#define VAS_HVWC_SIZE			512

But then we map it, which will round up to a page anyway. So again I
don't see the point of having the len returned form this helper.

> +}
> +
> +static inline void get_uwc_mmio_bar(struct vas_window *window,
> +			uint64_t *start, int *len)
> +{
> +	uint64_t pbaddr;
> +
> +	pbaddr = window->vinst->uwc_bar_start;
> +	*start = pbaddr + window->winid * VAS_UWC_SIZE;
> +	*len = VAS_UWC_SIZE;
> +}
> +
> +/*
> + * Map the paste bus address of the given send window into kernel address
> + * space. Unlike MMIO regions (map_mmio_region() below), paste region must
> + * be mapped cache-able and is only applicable to send windows.
> + */
> +void *map_paste_region(struct vas_window *txwin)
> +{
> +	int rc, len;
> +	void *map;
> +	char *name;
> +	uint64_t start;
> +
> +	rc = -ENOMEM;

You don't need that.

> +	name = kasprintf(GFP_KERNEL, "window-v%d-w%d", txwin->vinst->vas_id,
> +				txwin->winid);
> +	if (!name)
> +		return ERR_PTR(rc);

That can goto free_name;

> +
> +	txwin->paste_addr_name = name;
> +	compute_paste_address(txwin, &start, &len);
> +
> +	if (!request_mem_region(start, len, name)) {
> +		pr_devel("%s(): request_mem_region(0x%llx, %d) failed\n",
> +				__func__, start, len);
> +		goto free_name;
> +	}
> +
> +	map = ioremap_cache(start, len);
> +	if (!map) {
> +		pr_devel("%s(): ioremap_cache(0x%llx, %d) failed\n", __func__,
> +				start, len);
> +		goto free_name;
> +	}
> +
> +	pr_devel("VAS: mapped paste addr 0x%llx to kaddr 0x%p\n", start, map);
> +	return map;
> +
> +free_name:
> +	kfree(name);

Because kfree(NULL) is fine.

> +	return ERR_PTR(rc);

And that can just return ERR_PTR(-ENOMEM);

> +}

cheers

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v7 00/12] Enable VAS Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:40 +0200
  [PATCH v7 04/12] powerpc/vas: Define helpers to access MMIO regions Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:40 +0200
    Re: [PATCH v7 04/12] powerpc/vas: Define helpers to access MMIO regions Michael Ellerman <mpe@ellerman.id.au> - 2017-08-25 05:40 +0200
      Re: [PATCH v7 04/12] powerpc/vas: Define helpers to access MMIO  regions Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-28 06:40 +0200
  [PATCH v7 08/12] powerpc/vas: Define vas_win_id() Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:40 +0200
    Re: [PATCH v7 08/12] powerpc/vas: Define vas_win_id() Michael Ellerman <mpe@ellerman.id.au> - 2017-08-25 11:40 +0200
      Re: [PATCH v7 08/12] powerpc/vas: Define vas_win_id() Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-28 07:00 +0200
  [PATCH v7 01/12] powerpc/vas: Define macros, register fields and structures Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:40 +0200
    Re: [PATCH v7 01/12] powerpc/vas: Define macros, register fields and structures Michael Ellerman <mpe@ellerman.id.au> - 2017-08-25 11:50 +0200
  [PATCH v7 10/12] powerpc/vas: Define vas_win_close() interface Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:40 +0200
    Re: [PATCH v7 10/12] powerpc/vas: Define vas_win_close() interface Michael Ellerman <mpe@ellerman.id.au> - 2017-08-25 12:10 +0200
      Re: [PATCH v7 10/12] powerpc/vas: Define vas_win_close() interface Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-28 07:20 +0200
        Re: [PATCH v7 10/12] powerpc/vas: Define vas_win_close() interface Michael Ellerman <mpe@ellerman.id.au> - 2017-08-28 13:50 +0200
  [PATCH v7 05/12] powerpc/vas: Define helpers to init window context Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:50 +0200
    Re: [PATCH v7 05/12] powerpc/vas: Define helpers to init window context Michael Ellerman <mpe@ellerman.id.au> - 2017-08-25 11:30 +0200
      Re: [PATCH v7 05/12] powerpc/vas: Define helpers to init window  context Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-28 06:50 +0200
  [PATCH v7 09/12] powerpc/vas: Define vas_rx_win_open() interface Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:50 +0200
  [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit() Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:50 +0200
    Re: [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit() Michael Ellerman <mpe@ellerman.id.au> - 2017-08-24 14:00 +0200
      Re: [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit() Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 23:50 +0200
  [PATCH v7 02/12] Move GET_FIELD/SET_FIELD to vas.h Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> - 2017-08-24 08:50 +0200

csiph-web