[PATCH] stm: the number of masters should be (sw_end - sw_start + 1)

[PATCH] stm: the number of masters should be (sw_end - sw_start + 1)

[Chunyan Zhang]

sw_end represents the last software master, sw_start is index of the
first master, so the number of software masters should be
sw_end - sw_start + 1.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/intel_th/sth.c | 2 +-
 drivers/hwtracing/stm/core.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
index 56101c3..28917d7 100644
--- a/drivers/hwtracing/intel_th/sth.c
+++ b/drivers/hwtracing/intel_th/sth.c
@@ -173,7 +173,7 @@ static int intel_th_sw_init(struct sth_device *sth)
 	sth->stm.sw_start = reg & 0xffff;
 	sth->stm.sw_end = reg >> 16;
 
-	sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start;
+	sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start + 1;
 	dev_dbg(sth->dev, "sw_start: %x sw_end: %x masters: %x nchannels: %x\n",
 		sth->stm.sw_start, sth->stm.sw_end, sth->sw_nmasters,
 		sth->stm.sw_nchannels);
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 7f7bdb3..cb676f2 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -632,7 +632,7 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
 	if (!stm_data->packet || !stm_data->sw_nchannels)
 		return -EINVAL;
 
-	nmasters = stm_data->sw_end - stm_data->sw_start;
+	nmasters = stm_data->sw_end - stm_data->sw_start + 1;
 	stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);
 	if (!stm)
 		return -ENOMEM;
-- 
1.9.1

Re: [PATCH] stm: the number of masters should be (sw_end - sw_start + 1)

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> sw_end represents the last software master, sw_start is index of the
> first master, so the number of software masters should be
> sw_end - sw_start + 1.

Looks about right, but it needs to be in two separate patches.

> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/hwtracing/intel_th/sth.c | 2 +-
>  drivers/hwtracing/stm/core.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
> index 56101c3..28917d7 100644
> --- a/drivers/hwtracing/intel_th/sth.c
> +++ b/drivers/hwtracing/intel_th/sth.c
> @@ -173,7 +173,7 @@ static int intel_th_sw_init(struct sth_device *sth)
>  	sth->stm.sw_start = reg & 0xffff;
>  	sth->stm.sw_end = reg >> 16;
>  
> -	sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start;
> +	sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start + 1;
>  	dev_dbg(sth->dev, "sw_start: %x sw_end: %x masters: %x nchannels: %x\n",
>  		sth->stm.sw_start, sth->stm.sw_end, sth->sw_nmasters,
>  		sth->stm.sw_nchannels);
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index 7f7bdb3..cb676f2 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -632,7 +632,7 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
>  	if (!stm_data->packet || !stm_data->sw_nchannels)
>  		return -EINVAL;
>  
> -	nmasters = stm_data->sw_end - stm_data->sw_start;
> +	nmasters = stm_data->sw_end - stm_data->sw_start + 1;
>  	stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);

Or even offsetof(struct stm_device, masters[stm_data->sw_end]).

>  	if (!stm)
>  		return -ENOMEM;
> -- 
> 1.9.1

This is a very old version on git, btw. :)

Thanks,
--
Alex

Re: [PATCH] stm: the number of masters should be (sw_end - sw_start + 1)

[Chunyan Zhang]

On Fri, Dec 11, 2015 at 3:31 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> sw_end represents the last software master, sw_start is index of the
>> first master, so the number of software masters should be
>> sw_end - sw_start + 1.
>
> Looks about right, but it needs to be in two separate patches.

Will split this patch later.

>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  drivers/hwtracing/intel_th/sth.c | 2 +-
>>  drivers/hwtracing/stm/core.c     | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
>> index 56101c3..28917d7 100644
>> --- a/drivers/hwtracing/intel_th/sth.c
>> +++ b/drivers/hwtracing/intel_th/sth.c
>> @@ -173,7 +173,7 @@ static int intel_th_sw_init(struct sth_device *sth)
>>       sth->stm.sw_start = reg & 0xffff;
>>       sth->stm.sw_end = reg >> 16;
>>
>> -     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start;
>> +     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start + 1;
>>       dev_dbg(sth->dev, "sw_start: %x sw_end: %x masters: %x nchannels: %x\n",
>>               sth->stm.sw_start, sth->stm.sw_end, sth->sw_nmasters,
>>               sth->stm.sw_nchannels);
>> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
>> index 7f7bdb3..cb676f2 100644
>> --- a/drivers/hwtracing/stm/core.c
>> +++ b/drivers/hwtracing/stm/core.c
>> @@ -632,7 +632,7 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
>>       if (!stm_data->packet || !stm_data->sw_nchannels)
>>               return -EINVAL;
>>
>> -     nmasters = stm_data->sw_end - stm_data->sw_start;
>> +     nmasters = stm_data->sw_end - stm_data->sw_start + 1;
>>       stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);
>
> Or even offsetof(struct stm_device, masters[stm_data->sw_end]).

Ok, I can add this modification in next version, no need to make this
being a separated patch, right?

>
>>       if (!stm)
>>               return -ENOMEM;
>> --
>> 1.9.1
>
> This is a very old version on git, btw. :)

Didn't notice this before :), I have been using the one that Ubuntu
14.04 provided.

Thanks,
Chunyan

>
> Thanks,
> --
> Alex

Re: [PATCH] stm: the number of masters should be (sw_end - sw_start + 1)

[Chunyan Zhang]

On Fri, Dec 11, 2015 at 3:31 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> sw_end represents the last software master, sw_start is index of the
>> first master, so the number of software masters should be
>> sw_end - sw_start + 1.
>
> Looks about right, but it needs to be in two separate patches.
>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  drivers/hwtracing/intel_th/sth.c | 2 +-
>>  drivers/hwtracing/stm/core.c     | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
>> index 56101c3..28917d7 100644
>> --- a/drivers/hwtracing/intel_th/sth.c
>> +++ b/drivers/hwtracing/intel_th/sth.c
>> @@ -173,7 +173,7 @@ static int intel_th_sw_init(struct sth_device *sth)
>>       sth->stm.sw_start = reg & 0xffff;
>>       sth->stm.sw_end = reg >> 16;
>>
>> -     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start;
>> +     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start + 1;
>>       dev_dbg(sth->dev, "sw_start: %x sw_end: %x masters: %x nchannels: %x\n",
>>               sth->stm.sw_start, sth->stm.sw_end, sth->sw_nmasters,
>>               sth->stm.sw_nchannels);
>> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
>> index 7f7bdb3..cb676f2 100644
>> --- a/drivers/hwtracing/stm/core.c
>> +++ b/drivers/hwtracing/stm/core.c
>> @@ -632,7 +632,7 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
>>       if (!stm_data->packet || !stm_data->sw_nchannels)
>>               return -EINVAL;
>>
>> -     nmasters = stm_data->sw_end - stm_data->sw_start;
>> +     nmasters = stm_data->sw_end - stm_data->sw_start + 1;
>>       stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);
>
> Or even offsetof(struct stm_device, masters[stm_data->sw_end]).
>

This should use 'offsetofend()'.

Thanks,
Chunyan

>>       if (!stm)
>>               return -ENOMEM;
>> --
>> 1.9.1
>
> This is a very old version on git, btw. :)
>
> Thanks,
> --
> Alex

Re: [PATCH] stm: the number of masters should be (sw_end - sw_start + 1)

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> On Fri, Dec 11, 2015 at 3:31 PM, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>
>>> sw_end represents the last software master, sw_start is index of the
>>> first master, so the number of software masters should be
>>> sw_end - sw_start + 1.
>>
>> Looks about right, but it needs to be in two separate patches.
>>
>>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>>> ---
>>>  drivers/hwtracing/intel_th/sth.c | 2 +-
>>>  drivers/hwtracing/stm/core.c     | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
>>> index 56101c3..28917d7 100644
>>> --- a/drivers/hwtracing/intel_th/sth.c
>>> +++ b/drivers/hwtracing/intel_th/sth.c
>>> @@ -173,7 +173,7 @@ static int intel_th_sw_init(struct sth_device *sth)
>>>       sth->stm.sw_start = reg & 0xffff;
>>>       sth->stm.sw_end = reg >> 16;
>>>
>>> -     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start;
>>> +     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start + 1;
>>>       dev_dbg(sth->dev, "sw_start: %x sw_end: %x masters: %x nchannels: %x\n",
>>>               sth->stm.sw_start, sth->stm.sw_end, sth->sw_nmasters,
>>>               sth->stm.sw_nchannels);
>>> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
>>> index 7f7bdb3..cb676f2 100644
>>> --- a/drivers/hwtracing/stm/core.c
>>> +++ b/drivers/hwtracing/stm/core.c
>>> @@ -632,7 +632,7 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
>>>       if (!stm_data->packet || !stm_data->sw_nchannels)
>>>               return -EINVAL;
>>>
>>> -     nmasters = stm_data->sw_end - stm_data->sw_start;
>>> +     nmasters = stm_data->sw_end - stm_data->sw_start + 1;
>>>       stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);
>>
>> Or even offsetof(struct stm_device, masters[stm_data->sw_end]).
>>
>
> This should use 'offsetofend()'.

No, actually, just scratch my previous comment as it was completely
wrong, just fix the off-by-one. If we were to use offsetof(), it should
rather be of masters[nmasters], but all we need is to fix the off-by-one
right now.

Regards,
--
Alex

Re: [PATCH] stm: the number of masters should be (sw_end - sw_start + 1)

[Chunyan Zhang]

On Fri, Dec 11, 2015 at 4:51 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> On Fri, Dec 11, 2015 at 3:31 PM, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>>
>>>> sw_end represents the last software master, sw_start is index of the
>>>> first master, so the number of software masters should be
>>>> sw_end - sw_start + 1.
>>>
>>> Looks about right, but it needs to be in two separate patches.
>>>
>>>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>>>> ---
>>>>  drivers/hwtracing/intel_th/sth.c | 2 +-
>>>>  drivers/hwtracing/stm/core.c     | 2 +-
>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
>>>> index 56101c3..28917d7 100644
>>>> --- a/drivers/hwtracing/intel_th/sth.c
>>>> +++ b/drivers/hwtracing/intel_th/sth.c
>>>> @@ -173,7 +173,7 @@ static int intel_th_sw_init(struct sth_device *sth)
>>>>       sth->stm.sw_start = reg & 0xffff;
>>>>       sth->stm.sw_end = reg >> 16;
>>>>
>>>> -     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start;
>>>> +     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start + 1;
>>>>       dev_dbg(sth->dev, "sw_start: %x sw_end: %x masters: %x nchannels: %x\n",
>>>>               sth->stm.sw_start, sth->stm.sw_end, sth->sw_nmasters,
>>>>               sth->stm.sw_nchannels);
>>>> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
>>>> index 7f7bdb3..cb676f2 100644
>>>> --- a/drivers/hwtracing/stm/core.c
>>>> +++ b/drivers/hwtracing/stm/core.c
>>>> @@ -632,7 +632,7 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
>>>>       if (!stm_data->packet || !stm_data->sw_nchannels)
>>>>               return -EINVAL;
>>>>
>>>> -     nmasters = stm_data->sw_end - stm_data->sw_start;
>>>> +     nmasters = stm_data->sw_end - stm_data->sw_start + 1;
>>>>       stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);
>>>
>>> Or even offsetof(struct stm_device, masters[stm_data->sw_end]).
>>>
>>
>> This should use 'offsetofend()'.
>
> No, actually, just scratch my previous comment as it was completely
> wrong, just fix the off-by-one. If we were to use offsetof(), it should
> rather be of masters[nmasters], but all we need is to fix the off-by-one
> right now.
>

Sorry, you may lose me here, what's 'off-by-one' ?

Thanks,
Chunyan

> Regards,
> --
> Alex

Re: [PATCH] stm: the number of masters should be (sw_end - sw_start + 1)

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> On Fri, Dec 11, 2015 at 4:51 PM, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>
>>> On Fri, Dec 11, 2015 at 3:31 PM, Alexander Shishkin
>>> <alexander.shishkin@linux.intel.com> wrote:
>>>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>>>
>>>>> sw_end represents the last software master, sw_start is index of the
>>>>> first master, so the number of software masters should be
>>>>> sw_end - sw_start + 1.
>>>>
>>>> Looks about right, but it needs to be in two separate patches.
>>>>
>>>>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>>>>> ---
>>>>>  drivers/hwtracing/intel_th/sth.c | 2 +-
>>>>>  drivers/hwtracing/stm/core.c     | 2 +-
>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
>>>>> index 56101c3..28917d7 100644
>>>>> --- a/drivers/hwtracing/intel_th/sth.c
>>>>> +++ b/drivers/hwtracing/intel_th/sth.c
>>>>> @@ -173,7 +173,7 @@ static int intel_th_sw_init(struct sth_device *sth)
>>>>>       sth->stm.sw_start = reg & 0xffff;
>>>>>       sth->stm.sw_end = reg >> 16;
>>>>>
>>>>> -     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start;
>>>>> +     sth->sw_nmasters = sth->stm.sw_end - sth->stm.sw_start + 1;
>>>>>       dev_dbg(sth->dev, "sw_start: %x sw_end: %x masters: %x nchannels: %x\n",
>>>>>               sth->stm.sw_start, sth->stm.sw_end, sth->sw_nmasters,
>>>>>               sth->stm.sw_nchannels);
>>>>> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
>>>>> index 7f7bdb3..cb676f2 100644
>>>>> --- a/drivers/hwtracing/stm/core.c
>>>>> +++ b/drivers/hwtracing/stm/core.c
>>>>> @@ -632,7 +632,7 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
>>>>>       if (!stm_data->packet || !stm_data->sw_nchannels)
>>>>>               return -EINVAL;
>>>>>
>>>>> -     nmasters = stm_data->sw_end - stm_data->sw_start;
>>>>> +     nmasters = stm_data->sw_end - stm_data->sw_start + 1;
>>>>>       stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);
>>>>
>>>> Or even offsetof(struct stm_device, masters[stm_data->sw_end]).
>>>>
>>>
>>> This should use 'offsetofend()'.
>>
>> No, actually, just scratch my previous comment as it was completely
>> wrong, just fix the off-by-one. If we were to use offsetof(), it should
>> rather be of masters[nmasters], but all we need is to fix the off-by-one
>> right now.
>>
>
> Sorry, you may lose me here, what's 'off-by-one' ?

It's an error when the result of your calculation is off by 1 (one too
few or one too many).

https://www.google.com/search?q=off-by-one

Regards,
--
Alex