On Mon, 4 May 2015, Bryan O'Donoghue wrote:
> +++ b/arch/x86/include/asm/esram.h
This should be in platform/quark/....
> +++ b/arch/x86/platform/intel-quark/esram.c
> +#define esram_to_phys(x) ((x) << PAGE_SHIFT)
Unused macro.
> +#define phys_to_esram(x) ((x) >> PAGE_SHIFT)
There is a single usage size for this lousy documented magic.
> +/**
> + * struct esram_page
> + *
> + * Represents an eSRAM page.
> + */
> +struct esram_page {
> + u32 id;
> + struct list_head list;
> + phys_addr_t addr;
Please tab align the struct member names as you did below.
> +};
> +
> +/**
> + * struct esram_dev
> + *
> + * Structre to represent module state/data/etc.
> + */
> +struct esram_dev {
> + struct dentry *dbg;
So dbgfs is a hard requirement for this to work?
> + void *overlay;
> + struct esram_page *pages;
> + struct gen_pool *pool;
> + u8 cbuf[PAGE_SIZE];
> + bool init;
> + struct mutex lock;
> + u32 num_bytes;
> + struct list_head page_list;
> + u32 total_pages;
Lots of magic fields ....
> +};
> +
> +static struct esram_dev esram_dev;
> +
> +/**
> + * esram_dbgfs_state_show - print state of eSRAM registers.
> + *
> + * @s: pointer to seq_file for output.
> + * @unused: unused parameter.
> + * @return: 0 on success or error code passed from mbi_iosf on failure.
I don't think kerneldoc is happy about that one, but I might be wrong
> + */
> +static int esram_dbgfs_state_show(struct seq_file *s, void *unused)
> +{
> + struct esram_dev *edev = &esram_dev;
> + u32 data;
> + u32 reg = (u32)s->private;
You really like to waste lines. What's wrong with:
u32 data, reg = .....
> + int ret;
> +
> + mutex_lock(&edev->lock);
> + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, reg, &data);
> + if (ret == 0)
if (!ret)
> + seq_printf(s, "0x%08x\n", data);
> +/**
> + * esram_dump_fault - dump eSRAM registers and BUG().
> + *
> + * @return:
Sigh. Please generate kernel docs from your file to catch all those
function comment failures.
> + */
> +static void esram_dump_fault(struct esram_page *ep)
> +{
> + u32 pgc;
> + u32 pgd;
> + u32 pgb;
One line please for all of these.
> +
> + /* Show the page state. */
> + iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_READ, ep->id, &pgd);
> + pr_err("fault @ page %d state 0x%08x\n", ep->id, pgd);
> +
> + /* Get state. */
> + iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMCTRL_REG, &pgc);
> + iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMPGBLOCK_REG, &pgb);
> + pr_err("page-control=0x%08x, page-block=0x%08x\n", pgc, pgb);
> +
> + BUG();
So we force BUG() here. Why?
> +}
> +
> +/**
> + * esram_page_enable - Enable an eSRAM page spinning for page to become
> ready.
> + *
> + * @param ep: struct esram_page carries data to program to register.
> + * @return zero on success < 0 on error.
> + */
> +static int esram_page_enable(struct esram_page *ep)
> +{
> + int ret = 0;
> +
> + /* Enable a busy page => EINVAL, return IOSF error as necessary. */
Why is EINVAL a good return code if the page is busy?
> + ret = esram_page_busy(ep);
> + if (ret)
> + return ret < 0 ? ret : -EINVAL;
> +
> + /* Enable page overlay - with automatic flush on S3 entry. */
> + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_WRITE, ep->id,
> + ESRAMPGCTRL_FLUSH_PAGE_EN | ESRAMPGCTRL_EN |
> + phys_to_esram(ep->addr));
> + if (ret)
> + return ret;
> +
> + /* Busy bit true is good, ret < 0 means IOSF read error. */
> + ret = esram_page_busy(ep);
> + if (ret)
> + ret = 0;
> +
> + return ret;
Why not just return 0 unconitionally?
> +/**
> + * esram_page_overlay - Overlay a page with fast access eSRAM.
> + *
> + * This function takes a 4 KiB aligned physical address and programs an
> + * eSRAM page to overlay that 4 KiB region. We require and verify that the
> + * target memory is read-write - since we don't support overlay of read-only
> + * memory regions - such as kernel .text areas. Overlay of .text areas is
> + * not supported because eSRAM isn't self-populating and we cannot guarantee
> + * atomicity of the overlay operation. It is assumed and required that the
> + * caller of the overlay function is overlaying a data buffer not kernel
> + * code.
> + *
> + * @param ep: Pointer to eSRAM page desciptor.
> + * @return: 0 on success < 0 on failure.
> + */
> +static int esram_page_overlay(struct esram_dev *edev, struct esram_page *ep)
> +{
> + int level = 0;
> + void *vaddr = __va(ep->addr);
> + pte_t *pte = lookup_address((unsigned long)vaddr, &level);
> + int ret;
> +
> + /* We only support overlay for r/w memory. */
Well, the check below is not really checking for a valid memory
mapping. You can have a r/w PTE for some peripheral device space as
well.
> + if (pte == NULL || !(pte_write(*pte))) {
> + pr_err("invalid address for overlay %pa\n", &ep->addr);
> + return -ENOMEM;
> + }
Also what makes sure that the mapping is not going away under you?
Nothing, but the whole thing is not required at all. Because you map a
kernel buffer from init(), so these half baken sanity checks are
really useless.
> +
> + /* eSRAM does not autopopulate so save the contents. */
> + memcpy(&edev->cbuf, vaddr, PAGE_SIZE);
> + ret = esram_page_enable(ep);
> + if (ret) {
> + esram_dump_fault(ep);
> + goto err;
return ret perhaps?
> + }
> +
> + /* Overlay complete, repopulate the eSRAM page with original data. */
> + memcpy((void *)vaddr, &esram_dev.cbuf, PAGE_SIZE);
So the caller must ensure that the DRAM content cannot change between
the two memcpys, right? Otherwise you end up with inconsistent data.
At init() time I can see how that works, on resume() rather not.
> +err:
> + return ret;
> +}
> +
> +/**
> + * esram_map_page - Overlay a vritual address range aligned to 4 KiB.
> + *
> + * @param page: Page to map.
> + * @return: 0 success < 0 failure.
> + */
> +static int esram_map_page(struct esram_dev *edev, struct esram_page *ep)
> +{
> + int ret = 0;
> +
> + mutex_lock(&edev->lock);
> + ret = esram_page_overlay(edev, ep);
> + if (ret)
> + goto err;
Useless goto to further obfuscate the code.
> + list_add(&ep->list, &edev->page_list);
> +err:
> + mutex_unlock(&edev->lock);
> + return ret;
> +}
> +
> +/**
> + * esram_resume - restore eSRAM overlays on S3=>S0 transition.
> + *
> + * @return:
> + */
> +static void esram_resume(void)
> +{
> + struct esram_dev *edev = &esram_dev;
> + struct esram_page *ep = NULL;
> +
> + mutex_lock(&edev->lock);
> + list_for_each_entry(ep, &edev->page_list, list)
> + if (esram_page_overlay(edev, ep))
> + pr_err("restore page %d phys %pa fail!\n",
> + ep->id, &ep->addr);
> + mutex_unlock(&edev->lock);
> +}
> +
> +/* Shutdown is done by RMU. Kernel needs to-do the resume() though. */
> +static struct syscore_ops esram_syscore_ops = {
> + .resume = esram_resume,
> +};
> +
> +/**
> + * esram_get_genpool - return pointer to esram genpool structure.
> + *
> + * @return:
> + */
> +struct gen_pool *esram_get_genpool(void)
> +{
> + struct esram_dev *edev = &esram_dev;
> +
> + return edev->init ? edev->pool : NULL;
> +}
> +EXPORT_SYMBOL_GPL(esram_get_genpool);
> +
> +static const struct x86_cpu_id esram_ids[] __initconst = {
> + { X86_VENDOR_INTEL, 5, 9 }, /* Intel Quark SoC X1000. */
> + {}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, esram_ids);
> +
> + /**
> + * esram_init - entry point for eSRAM driver.
> + *
> + * This driver manages eSRAM on a per-page basis. Therefore if we find block
> + * mode is enabled, or any global, block-level or page-level locks are in
> place
> + * at module initialisation time - we bail out.
> + *
> + * return: -ENODEV for no eSRAM support 0 if good to go.
> + */
> +static int __init esram_init(void)
> +{
> + u32 block;
> + u32 ctrl;
> + struct esram_page *ep = NULL;
> + struct esram_dev *edev = &esram_dev;
> + phys_addr_t addr;
> + int i;
> + int ret;
> +
If there is a scheme in your variable declaration blocks, I rather
don't want to know about it.
> + /* Calculate # of pages silicon supports. */
> + edev->num_bytes = ESRAMCTRL_SIZE(ctrl);
> + edev->total_pages = edev->num_bytes / PAGE_SIZE;
> + if (edev->total_pages == 0)
> + return -ENOMEM;
> +
> + /* Get an array of esram pages. */
> + edev->pages = kzalloc(edev->total_pages *
> + sizeof(struct esram_page), GFP_KERNEL);
> + if (IS_ERR(edev->pages)) {
> + ret = PTR_ERR(edev->pages);
> + goto err;
> + }
> +
> + /* Make an area for the gen_pool to operate from. */
> + edev->overlay = kmalloc(edev->num_bytes, GFP_KERNEL);
This better be page aligned, right? How's that guaranteed?
> + /* Overlay contiguous region with eSRAM pages. */
> + addr = __pa(edev->overlay);
> + for (i = 0; i < edev->total_pages; i++) {
> + ep = &edev->pages[i];
> + ep->id = i;
> + ep->addr = addr;
> +
> + /* Validate page state is not busy. */
> + ret = esram_page_busy(ep);
> + if (ret) {
> + esram_dump_fault(ep);
> + ret = ret < 0 ? ret : -ENOMEM;
This return value juggling is really horrible and hard to follow.
> + goto err;
> + }
> +
> + /* Overlay. */
> + ret = esram_map_page(edev, ep);
> + if (ret)
> + goto err;
What undoes already established mappings?
> +static void __exit esram_exit(void)
> +{
> + struct esram_dev *edev = &esram_dev;
Again. What happens to the mappings?
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
