Re: [PATCH 01/11 v6] coresight: add CoreSight core layer framework

From: Mathieu Poirier
Date: Tue Sep 16 2014 - 15:43:01 EST


On 12 September 2014 12:44, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> Further to Greg's comments...
>
> On Thu, Sep 11, 2014 at 09:49:08AM -0600, mathieu.poirier@xxxxxxxxxx wrote:
>> +int coresight_enable(struct coresight_device *csdev)
>> +{
>> + int ret = 0;
>> + LIST_HEAD(path);
>> +
>> + WARN_ON(IS_ERR_OR_NULL(csdev));
>
> Please don't do this kind of checking, it just makes stuff a lot more
> noisy than it needs to be, and it doesn't give any value what so ever.
>
> I've seen code where it seems the coding style required that quite
> literally every function does extensive checking of function arguments,
> and every function returns a status. This does nothing to stop bugs.
> In fact, it makes things a /lot/ worse because there is then soo much
> junk in every function that you can't read what the function is doing
> anymore. Imagine memset() validating its arguments and returning a
> status value... I kid not.
>
> The point here is that if a NULL pointer is passed to this function,
> the above WARN_ON gets triggered, and we get a backtrace. We then
> continue on, take the semaphore, and then dereference the NULL pointer
> causing another backtrace. So the WARN_ON was utterly pointless.
>
> Just reference the pointer and don't bother with these silly checks.

Very well.

>
>> +
>> + down(&coresight_semaphore);
>
> What is your reason for using a semaphore rather than a mutex?

Thanks for insisting on this - after doing a little more research on
the topic I will be moving to a mutex implementation.

>
> ...
>> +/**
>> + * coresight_timeout - loop until a bit has changed to a specific state.
>> + * @addr: base address of the area of interest.
>> + * @offset: address of a register, starting from @addr.
>> + * @position: the position of the bit of interest.
>> + * @value: the value the bit should have.
>> + *
>> + * Returns as soon as the bit has taken the desired state or TIMEOU_US has
>
> Typo?

Indeed.

>
>> + * elapsed, which ever happens first.
>> + */
>> +
>> +void coresight_timeout(void __iomem *addr, u32 offset, int position, int value)
>> +{
>> + int i;
>> + u32 val;
>> +
>> + for (i = TIMEOUT_US; i > 0; i--) {
>> + val = __raw_readl(addr + offset);
>> + /* waiting on the bit to go from 0 to 1 */
>> + if (value) {
>> + if (val & BIT(position))
>> + return;
>> + /* waiting on the bit to go from 1 to 0 */
>> + } else {
>> + if (!(val & BIT(position)))
>> + return;
>> + }
>> +
>> + /* The specification doesn't say how long we are expected
>> + * to wait.
>> + */
>
> /*
> * The kernel commeting style for multi-line comments is
> * like this. Note the line opening the comment has no
> * comment text.
> */
>
>> + udelay(1);
>
> So the duration is arbitary.

Correct.

>
>> + }
>> +
>> + WARN(1,
>> + "coresight: timeout observed when proving at offset %#x\n",
>> + offset);
>
> This is also buggy. On the last loop iteration, we delay 1us, decrement
> i, and then test for it being greater than zero. If it isn't, print
> this message and do a backtrace (why is a backtrace useful here?)
> The important point here is that we waited for 1us, and /didn't/ test
> for success before claiming timeout. That makes the final 1us wait
> entirely useless.

Good catch.

>
>> diff --git a/drivers/coresight/of_coresight.c b/drivers/coresight/of_coresight.c
>> new file mode 100644
>> index 0000000..c780b4b
>> --- /dev/null
>> +++ b/drivers/coresight/of_coresight.c
>> @@ -0,0 +1,201 @@
> ...
>> +struct coresight_platform_data *of_get_coresight_platform_data(
>> + struct device *dev, struct device_node *node)
>> +{
>> + int id, i = 0, ret = 0;
>> + struct device_node *cpu;
>> + struct coresight_platform_data *pdata;
>> + struct of_endpoint endpoint, rendpoint;
>> + struct device_node *ep = NULL;
>> + struct device_node *rparent = NULL;
>> + struct device_node *rport = NULL;
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata)
>> + return ERR_PTR(-ENOMEM);
>
> So, what are the rules for calling this function? What is the expected
> lifetime of this pdata structure in relation to 'dev' ?
>
> You do realise that when a driver unbinds from 'dev', this allocation
> will be freed. If you hold on to the pointer and dereference it, you
> could be accessing memory allocated for other purposes at that point.

By the time the allocation of pdata is freed the driver will have
called "coresight_unregister()", removing all knowledge of that entity
by the framework.

>
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> new file mode 100644
>> index 0000000..5b22287
>> --- /dev/null
>> +++ b/include/linux/coresight.h
>> @@ -0,0 +1,275 @@
> ...
>> +/**
>> + * @name: name of the entry to appear under the component's
>> + debugfs sub-directory.
>> + * @mode: what operation can be performed on the entry.
>> + * @ops: specific manipulation to be done using this entry.
>> + */
>
> Wrong commenting style. Please read Documentation/kernel-doc-nano-HOWTO.txt
> for information how to document structures.

Ok.

>
> --
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/