lkml.org 
[lkml]   [2009]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/6] omap iommu: tlb and pagetable primitives
    A few more simple comments...

    On Fri, Jan 16, 2009 at 10:37:09AM +0200, Hiroshi DOYU wrote:
    > +struct iommu_platform_data {
    > + char *name;

    const?

    > + char *clk_name;

    const?

    > + int nr_tlb_entries;
    > +};

    ...

    > +int install_iommu_arch(const struct iommu_functions *ops)
    > +{
    > + if (arch_iommu)
    > + return -EBUSY;
    > +
    > + arch_iommu = ops;
    > + return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(install_iommu_arch);

    Exported functions should have some documentation in the standard kernel
    documentation format. See Documentation/kernel-doc-nano-HOWTO.txt

    > +static int iommu_enable(struct iommu *obj)
    > +{
    > + int err;
    > +
    > + BUG_ON(!arch_iommu || !arch_iommu->enable);

    Are these run-time BUG_ON checks really worth it? BUG_ON is effectively:

    if (condition_is_true)
    cause_a_null_pointer_dereference;

    So, if you're checking for pointers you're about dereference being NULL,
    there's little point - dereferencing the when they're NULL will cause an
    oops, and you won't have the overhead of the runtime tests checking them
    for NULL. Ditto elsewhere in this file.

    > +static u32 *iopte_alloc(struct iommu *obj, u32 *iopgd, u32 da)
    > +{
    > + u32 *iopte;
    > +
    > + /* a table has already existed */
    > + if (*iopgd)
    > + goto pte_ready;
    > +
    > + /*
    > + * do the allocation outside the page table lock
    > + */
    > + spin_unlock(&obj->page_table_lock);
    > + iopte = kmem_cache_zalloc(iopte_cachep, GFP_KERNEL);
    > + spin_lock(&obj->page_table_lock);
    > +
    > + if (!*iopgd) {
    > + if (!iopte)
    > + return ERR_PTR(-ENOMEM);
    > +
    > + *iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
    > + flush_iopgd_range(iopgd, iopgd);
    > +
    > +#ifdef DEBUG_VERBOSE
    > + dev_dbg(obj->dev, "%s:\ta new pte:%p\n", __func__, iopte);
    > +#endif

    Not sure wrapping these in DEBUG_VERBOSE is necessary. dev_dbg() is a
    no-op unless DEBUG is defined.

    > +#ifdef DEBUG
    > +static void dump_tlb_entries(struct iommu *obj)
    > +{
    > + int i;
    > + struct iotlb_lock l;
    > +
    > + clk_enable(obj->clk);
    > +
    > + pr_info("%8s %8s\n", "cam:", "ram:");
    > + pr_info("-----------------------------------------\n");
    > +
    > + for (i = 0; i < obj->nr_tlb_entries; i++) {
    > + struct cr_regs cr;
    > + static char buf[4096];
    > +
    > + iotlb_lock_get(obj, &l);
    > + l.vict = i;
    > + iotlb_lock_set(obj, &l);
    > + iotlb_read_cr(obj, &cr);
    > + if (!iotlb_cr_valid(&cr))
    > + continue;
    > +
    > + memset(buf, 0, 4096);
    > + iotlb_dump_cr(obj, &cr, buf);
    > + pr_info("%s", buf);

    Hmm. You call this in relation to an error, but you print everything at
    'info' level. Are you sure that's correct?

    > +/*
    > + * OMAP Device MMU(IOMMU) detection
    > + */
    > +static int __devinit omap_iommu_probe(struct platform_device *pdev)
    > +{
    > + int err = -ENODEV;
    > + void *p;
    > + int irq;
    > + struct iommu *obj;
    > + struct resource *res;
    > + struct iommu_platform_data *pdata = pdev->dev.platform_data;
    > +
    > + if (pdev->num_resources != 2)
    > + return -EINVAL;
    > +
    > + obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
    > + if (!obj)
    > + return -ENOMEM;
    > +
    > + obj->clk = clk_get(NULL, pdata->clk_name);

    Avoid passing a NULL struct device except when you have absolutely no other
    choice. &pdev->dev looks sensible. By doing this, it also provides us with
    a path to fixing the OMAP clk API implementation without having to fix lots
    of drivers.


    \
     
     \ /
      Last update: 2009-01-17 17:09    [W:0.031 / U:31.812 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site