[dpdk-dev] net/szedata2: fix incorrect device memory access
Checks
Commit Message
Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
Signed-off-by: Matej Vido <vido@cesnet.cz>
---
drivers/net/szedata2/rte_eth_szedata2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 1/24/2017 10:49 AM, Matej Vido wrote:
> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>
> Signed-off-by: Matej Vido <vido@cesnet.cz>
Unrelated from this patch, in maintainers file, you have your other mail
address: "Matej Vido <matejvido@gmail.com>", do you want to update it?
> ---
> drivers/net/szedata2/rte_eth_szedata2.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/szedata2/rte_eth_szedata2.h b/drivers/net/szedata2/rte_eth_szedata2.h
> index b58adb6..afe8a38 100644
> --- a/drivers/net/szedata2/rte_eth_szedata2.h
> +++ b/drivers/net/szedata2/rte_eth_szedata2.h
> @@ -192,7 +192,7 @@ struct szedata {
> }
>
> #define SZEDATA2_PCI_RESOURCE_PTR(rsc, offset, type) \
> - ((type)((uint8_t *)(rsc)->addr) + (offset))
> + ((type)(((uint8_t *)(rsc)->addr) + (offset)))
Although output will be same, (in all uses, type is a pointer), this
seems the intention, so:
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
btw, following will do same, right, not sure if it is better:
((type)(rsc)->addr + (offset))
>
> enum szedata2_link_speed {
> SZEDATA2_LINK_SPEED_DEFAULT = 0,
>
On 24.01.2017 12:58, Ferruh Yigit wrote:
> On 1/24/2017 10:49 AM, Matej Vido wrote:
>> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>>
>> Signed-off-by: Matej Vido <vido@cesnet.cz>
> Unrelated from this patch, in maintainers file, you have your other mail
> address: "Matej Vido <matejvido@gmail.com>", do you want to update it?
Hi Ferruh,
yes, I will send the patch.
>
>> ---
>> drivers/net/szedata2/rte_eth_szedata2.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/szedata2/rte_eth_szedata2.h b/drivers/net/szedata2/rte_eth_szedata2.h
>> index b58adb6..afe8a38 100644
>> --- a/drivers/net/szedata2/rte_eth_szedata2.h
>> +++ b/drivers/net/szedata2/rte_eth_szedata2.h
>> @@ -192,7 +192,7 @@ struct szedata {
>> }
>>
>> #define SZEDATA2_PCI_RESOURCE_PTR(rsc, offset, type) \
>> - ((type)((uint8_t *)(rsc)->addr) + (offset))
>> + ((type)(((uint8_t *)(rsc)->addr) + (offset)))
> Although output will be same, (in all uses, type is a pointer), this
> seems the intention, so:
>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
> btw, following will do same, right, not sure if it is better:
> ((type)(rsc)->addr + (offset))
This is also wrong. The intention of the macro is to add an offset to
the base address and typecast the result.
Regards,
Matej
>
>>
>> enum szedata2_link_speed {
>> SZEDATA2_LINK_SPEED_DEFAULT = 0,
>>
On 1/24/2017 2:02 PM, Matej Vido wrote:
> On 24.01.2017 12:58, Ferruh Yigit wrote:
>> On 1/24/2017 10:49 AM, Matej Vido wrote:
>>> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>>>
>>> Signed-off-by: Matej Vido <vido@cesnet.cz>
>> Unrelated from this patch, in maintainers file, you have your other mail
>> address: "Matej Vido <matejvido@gmail.com>", do you want to update it?
> Hi Ferruh,
>
> yes, I will send the patch.
>>
>>> ---
>>> drivers/net/szedata2/rte_eth_szedata2.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/szedata2/rte_eth_szedata2.h b/drivers/net/szedata2/rte_eth_szedata2.h
>>> index b58adb6..afe8a38 100644
>>> --- a/drivers/net/szedata2/rte_eth_szedata2.h
>>> +++ b/drivers/net/szedata2/rte_eth_szedata2.h
>>> @@ -192,7 +192,7 @@ struct szedata {
>>> }
>>>
>>> #define SZEDATA2_PCI_RESOURCE_PTR(rsc, offset, type) \
>>> - ((type)((uint8_t *)(rsc)->addr) + (offset))
>>> + ((type)(((uint8_t *)(rsc)->addr) + (offset)))
>> Although output will be same, (in all uses, type is a pointer), this
>> seems the intention, so:
>>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>
>> btw, following will do same, right, not sure if it is better:
>> ((type)(rsc)->addr + (offset))
> This is also wrong. The intention of the macro is to add an offset to
> the base address and typecast the result.
Right, again this will give same output when "type" is pointer, but
wrong for described intention.
>
> Regards,
> Matej
>>
>>>
>>> enum szedata2_link_speed {
>>> SZEDATA2_LINK_SPEED_DEFAULT = 0,
>>>
>
On 1/24/2017 2:02 PM, Matej Vido wrote:
> On 24.01.2017 12:58, Ferruh Yigit wrote:
>> On 1/24/2017 10:49 AM, Matej Vido wrote:
>>> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>>>
>>> Signed-off-by: Matej Vido <vido@cesnet.cz>
>> Unrelated from this patch, in maintainers file, you have your other mail
>> address: "Matej Vido <matejvido@gmail.com>", do you want to update it?
> Hi Ferruh,
>
> yes, I will send the patch.
>>
>>> ---
>>> drivers/net/szedata2/rte_eth_szedata2.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/szedata2/rte_eth_szedata2.h b/drivers/net/szedata2/rte_eth_szedata2.h
>>> index b58adb6..afe8a38 100644
>>> --- a/drivers/net/szedata2/rte_eth_szedata2.h
>>> +++ b/drivers/net/szedata2/rte_eth_szedata2.h
>>> @@ -192,7 +192,7 @@ struct szedata {
>>> }
>>>
>>> #define SZEDATA2_PCI_RESOURCE_PTR(rsc, offset, type) \
>>> - ((type)((uint8_t *)(rsc)->addr) + (offset))
>>> + ((type)(((uint8_t *)(rsc)->addr) + (offset)))
>> Although output will be same, (in all uses, type is a pointer),
Of course won't be same, please forget about it J, I am confused.
So these two differs a lot, taking into account that offset numbers used
are big numbers (0x8000..), it should be accessing very unrelated addresses.
So how this was working before?
> this
>> seems the intention, so:
>>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>
>> btw, following will do same, right, not sure if it is better:
>> ((type)(rsc)->addr + (offset))
> This is also wrong. The intention of the macro is to add an offset to
> the base address and typecast the result.
>
> Regards,
> Matej
>>
>>>
>>> enum szedata2_link_speed {
>>> SZEDATA2_LINK_SPEED_DEFAULT = 0,
>>>
>
On 1/24/2017 10:49 AM, Matej Vido wrote:
> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>
> Signed-off-by: Matej Vido <vido@cesnet.cz>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: stable@dpdk.org
Applied to dpdk-next-net/master, thanks.
On 24.01.2017 16:11, Ferruh Yigit wrote:
> On 1/24/2017 2:02 PM, Matej Vido wrote:
>> On 24.01.2017 12:58, Ferruh Yigit wrote:
>>> On 1/24/2017 10:49 AM, Matej Vido wrote:
>>>> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>>>>
>>>> Signed-off-by: Matej Vido <vido@cesnet.cz>
>>> Unrelated from this patch, in maintainers file, you have your other mail
>>> address: "Matej Vido <matejvido@gmail.com>", do you want to update it?
>> Hi Ferruh,
>>
>> yes, I will send the patch.
>>>> ---
>>>> drivers/net/szedata2/rte_eth_szedata2.h | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/szedata2/rte_eth_szedata2.h b/drivers/net/szedata2/rte_eth_szedata2.h
>>>> index b58adb6..afe8a38 100644
>>>> --- a/drivers/net/szedata2/rte_eth_szedata2.h
>>>> +++ b/drivers/net/szedata2/rte_eth_szedata2.h
>>>> @@ -192,7 +192,7 @@ struct szedata {
>>>> }
>>>>
>>>> #define SZEDATA2_PCI_RESOURCE_PTR(rsc, offset, type) \
>>>> - ((type)((uint8_t *)(rsc)->addr) + (offset))
>>>> + ((type)(((uint8_t *)(rsc)->addr) + (offset)))
>>> Although output will be same, (in all uses, type is a pointer),
> Of course won't be same, please forget about it J, I am confused.
>
> So these two differs a lot, taking into account that offset numbers used
> are big numbers (0x8000..), it should be accessing very unrelated addresses.
>
> So how this was working before?
The macro was fine before the patch [1]. It hasn't been working since
the acceptance of that patch, but I didn't manage to test the
functionality until now.
[1] http://dpdk.org/ml/archives/dev/2016-December/053241.html
Regards,
Matej
>
>> this
>>> seems the intention, so:
>>>
>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>
>>> btw, following will do same, right, not sure if it is better:
>>> ((type)(rsc)->addr + (offset))
>> This is also wrong. The intention of the macro is to add an offset to
>> the base address and typecast the result.
>>
>> Regards,
>> Matej
>>>>
>>>> enum szedata2_link_speed {
>>>> SZEDATA2_LINK_SPEED_DEFAULT = 0,
>>>>
On 1/24/2017 3:55 PM, Matej Vido wrote:
> On 24.01.2017 16:11, Ferruh Yigit wrote:
>> On 1/24/2017 2:02 PM, Matej Vido wrote:
>>> On 24.01.2017 12:58, Ferruh Yigit wrote:
>>>> On 1/24/2017 10:49 AM, Matej Vido wrote:
>>>>> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>>>>>
>>>>> Signed-off-by: Matej Vido <vido@cesnet.cz>
>>>> Unrelated from this patch, in maintainers file, you have your other mail
>>>> address: "Matej Vido <matejvido@gmail.com>", do you want to update it?
>>> Hi Ferruh,
>>>
>>> yes, I will send the patch.
>>>>> ---
>>>>> drivers/net/szedata2/rte_eth_szedata2.h | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/szedata2/rte_eth_szedata2.h b/drivers/net/szedata2/rte_eth_szedata2.h
>>>>> index b58adb6..afe8a38 100644
>>>>> --- a/drivers/net/szedata2/rte_eth_szedata2.h
>>>>> +++ b/drivers/net/szedata2/rte_eth_szedata2.h
>>>>> @@ -192,7 +192,7 @@ struct szedata {
>>>>> }
>>>>>
>>>>> #define SZEDATA2_PCI_RESOURCE_PTR(rsc, offset, type) \
>>>>> - ((type)((uint8_t *)(rsc)->addr) + (offset))
>>>>> + ((type)(((uint8_t *)(rsc)->addr) + (offset)))
>>>> Although output will be same, (in all uses, type is a pointer),
>> Of course won't be same, please forget about it J, I am confused.
>>
>> So these two differs a lot, taking into account that offset numbers used
>> are big numbers (0x8000..), it should be accessing very unrelated addresses.
>>
>> So how this was working before?
>
> The macro was fine before the patch [1]. It hasn't been working since
> the acceptance of that patch, but I didn't manage to test the
> functionality until now.
I see, thanks for clarification, seems broken relatively new, and thanks
for fixing.
>
> [1] http://dpdk.org/ml/archives/dev/2016-December/053241.html
>
> Regards,
> Matej
>
>>
>>> this
>>>> seems the intention, so:
>>>>
>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>
>>>> btw, following will do same, right, not sure if it is better:
>>>> ((type)(rsc)->addr + (offset))
>>> This is also wrong. The intention of the macro is to add an offset to
>>> the base address and typecast the result.
>>>
>>> Regards,
>>> Matej
>>>>>
>>>>> enum szedata2_link_speed {
>>>>> SZEDATA2_LINK_SPEED_DEFAULT = 0,
>>>>>
>
On 24.01.2017 16:24, Ferruh Yigit wrote:
> On 1/24/2017 10:49 AM, Matej Vido wrote:
>> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>>
>> Signed-off-by: Matej Vido <vido@cesnet.cz>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
> Cc: stable@dpdk.org
>
> Applied to dpdk-next-net/master, thanks.
I'm not sure about the policy regarding the stable releases, but I think
that this patch doesn't belong to stable as the bug was introduced and
also fixed during the current 17.02 window. Or am I wrong?
Thanks,
Matej
On 1/24/2017 4:05 PM, Matej Vido wrote:
> On 24.01.2017 16:24, Ferruh Yigit wrote:
>> On 1/24/2017 10:49 AM, Matej Vido wrote:
>>> Fixes: 8acba705b119 ("net/szedata2: localize handling of PCI resources")
>>>
>>> Signed-off-by: Matej Vido <vido@cesnet.cz>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>
>> Cc: stable@dpdk.org
>>
>> Applied to dpdk-next-net/master, thanks.
> I'm not sure about the policy regarding the stable releases, but I think
> that this patch doesn't belong to stable as the bug was introduced and
> also fixed during the current 17.02 window. Or am I wrong?
Yes you are right, I missed that issue introduced in this release, I
removed stable tag from commit log.
Thanks,
ferruh
>
> Thanks,
> Matej
>
@@ -192,7 +192,7 @@ struct szedata {
}
#define SZEDATA2_PCI_RESOURCE_PTR(rsc, offset, type) \
- ((type)((uint8_t *)(rsc)->addr) + (offset))
+ ((type)(((uint8_t *)(rsc)->addr) + (offset)))
enum szedata2_link_speed {
SZEDATA2_LINK_SPEED_DEFAULT = 0,