diff mbox series

[v8,1/6] software node: rename is_array to is_inline

Message ID 20191108042225.45391-2-dmitry.torokhov@gmail.com (mailing list archive)
State Deferred, archived
Headers show
Series software node: add support for reference properties | expand

Commit Message

Dmitry Torokhov Nov. 8, 2019, 4:22 a.m. UTC
We do not need a special flag to know if we are dealing with an array,
as we can get that data from ratio between element length and the data
size, however we do need a flag to know whether the data is stored
directly inside property_entry or separately.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 12 +++++-------
 include/linux/property.h | 13 ++++++++-----
 2 files changed, 13 insertions(+), 12 deletions(-)

Comments

Rafael J. Wysocki Nov. 8, 2019, 9:49 a.m. UTC | #1
On Fri, Nov 8, 2019 at 5:22 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> We do not need a special flag to know if we are dealing with an array,
> as we can get that data from ratio between element length and the data
> size, however we do need a flag to know whether the data is stored
> directly inside property_entry or separately.

So the subject is slightly misleading, because it is not a rename.  I
would say "replace x with y" instead.

[Arguably I can change that when applying the patch, but since we are
going to wait for the dependencies to go in, it should not be a big
deal to send an update of this patch alone?]

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/swnode.c    | 12 +++++-------
>  include/linux/property.h | 13 ++++++++-----
>  2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index d8d0dc0ca5acf..18a30fb3cc588 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
>         if (!prop->length)
>                 return NULL;
>
> -       if (prop->is_array)
> -               return prop->pointer;
> -
> -       return &prop->value;
> +       return prop->is_inline ? &prop->value : prop->pointer;
>  }
>
>  static const void *property_entry_find(const struct property_entry *props,
> @@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
>         const char * const *src_str;
>         size_t i, nval;
>
> -       if (p->is_array) {
> +       if (!p->is_inline) {
>                 if (p->type == DEV_PROP_STRING && p->pointer) {
>                         src_str = p->pointer;
>                         nval = p->length / sizeof(const char *);
> @@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
>         const void *pointer = property_get_pointer(src);
>         const void *new;
>
> -       if (src->is_array) {
> +       if (!src->is_inline) {
>                 if (!src->length)
>                         return -ENODATA;
>
> @@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
>                                 return -ENOMEM;
>                 }
>
> -               dst->is_array = true;
>                 dst->pointer = new;
>         } else if (src->type == DEV_PROP_STRING) {
>                 new = kstrdup(src->value.str, GFP_KERNEL);
>                 if (!new && src->value.str)
>                         return -ENOMEM;
>
> +               dst->is_inline = true;
>                 dst->value.str = new;
>         } else {
> +               dst->is_inline = true;
>                 dst->value = src->value;
>         }
>
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 48335288c2a96..dad0ad11b55e2 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
>   * struct property_entry - "Built-in" device property representation.
>   * @name: Name of the property.
>   * @length: Length of data making up the value.
> - * @is_array: True when the property is an array.
> + * @is_inline: True when the property value is embedded in
> + *     &struct property_entry instance.
>   * @type: Type of the data in unions.
> - * @pointer: Pointer to the property (an array of items of the given type).
> - * @value: Value of the property (when it is a single item of the given type).
> + * @pointer: Pointer to the property when it is stored separately from
> + *     the &struct property_entry instance.
> + * @value: Value of the property when it is stored inline.

And while at it, can you please try to make the comments shorter so
they each take one line?
Bjørn Mork Nov. 13, 2019, 6:52 a.m. UTC | #2
Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> We do not need a special flag to know if we are dealing with an array,
> as we can get that data from ratio between element length and the data
> size, however we do need a flag to know whether the data is stored
> directly inside property_entry or separately.

Doesn't a non-null prop->pointer tell you this?

And inverting the flag is unnecessarily risky IMHO. An all-zero prop
might now result in dereferencing a NULL prop->pointer instead of using
the empty prop->value.  Now I haven't looked at the code to see if this
is a real problem.  But I believe it's better not having to do that
anyway...


Bjørn
Dmitry Torokhov Nov. 13, 2019, 8:08 a.m. UTC | #3
On Wed, Nov 13, 2019 at 07:52:43AM +0100, Bjørn Mork wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> 
> > We do not need a special flag to know if we are dealing with an array,
> > as we can get that data from ratio between element length and the data
> > size, however we do need a flag to know whether the data is stored
> > directly inside property_entry or separately.
> 
> Doesn't a non-null prop->pointer tell you this?

No it does not because pointer is a part of a union.

> 
> And inverting the flag is unnecessarily risky IMHO. An all-zero prop
> might now result in dereferencing a NULL prop->pointer instead of using
> the empty prop->value.  Now I haven't looked at the code to see if this
> is a real problem.  But I believe it's better not having to do that
> anyway...

All-zero property is a terminator and thus we will not dereference
anything.

Thanks.
Marek Szyprowski Dec. 12, 2019, 11:12 a.m. UTC | #4
Dear All,

On 08.11.2019 05:22, Dmitry Torokhov wrote:
> We do not need a special flag to know if we are dealing with an array,
> as we can get that data from ratio between element length and the data
> size, however we do need a flag to know whether the data is stored
> directly inside property_entry or separately.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Today I've noticed that this patch got merged to linux-next as commit 
e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI 
driver operation on Samsung Exynos5 SoCs (and probably on other SoCs 
which use DWC3 in host mode too). I get the following errors during boot:

dwc3 12000000.dwc3: failed to add properties to xHCI
dwc3 12000000.dwc3: failed to initialize host
dwc3: probe of 12000000.dwc3 failed with error -61

Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:

https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt

(lack of 'ref' clk is not related nor fatal to the driver operation).

The code which fails after this patch is located in 
drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

> ---
>   drivers/base/swnode.c    | 12 +++++-------
>   include/linux/property.h | 13 ++++++++-----
>   2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index d8d0dc0ca5acf..18a30fb3cc588 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -108,10 +108,7 @@ static const void *property_get_pointer(const struct property_entry *prop)
>   	if (!prop->length)
>   		return NULL;
>   
> -	if (prop->is_array)
> -		return prop->pointer;
> -
> -	return &prop->value;
> +	return prop->is_inline ? &prop->value : prop->pointer;
>   }
>   
>   static const void *property_entry_find(const struct property_entry *props,
> @@ -205,7 +202,7 @@ static void property_entry_free_data(const struct property_entry *p)
>   	const char * const *src_str;
>   	size_t i, nval;
>   
> -	if (p->is_array) {
> +	if (!p->is_inline) {
>   		if (p->type == DEV_PROP_STRING && p->pointer) {
>   			src_str = p->pointer;
>   			nval = p->length / sizeof(const char *);
> @@ -250,7 +247,7 @@ static int property_entry_copy_data(struct property_entry *dst,
>   	const void *pointer = property_get_pointer(src);
>   	const void *new;
>   
> -	if (src->is_array) {
> +	if (!src->is_inline) {
>   		if (!src->length)
>   			return -ENODATA;
>   
> @@ -264,15 +261,16 @@ static int property_entry_copy_data(struct property_entry *dst,
>   				return -ENOMEM;
>   		}
>   
> -		dst->is_array = true;
>   		dst->pointer = new;
>   	} else if (src->type == DEV_PROP_STRING) {
>   		new = kstrdup(src->value.str, GFP_KERNEL);
>   		if (!new && src->value.str)
>   			return -ENOMEM;
>   
> +		dst->is_inline = true;
>   		dst->value.str = new;
>   	} else {
> +		dst->is_inline = true;
>   		dst->value = src->value;
>   	}
>   
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 48335288c2a96..dad0ad11b55e2 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -227,15 +227,17 @@ static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
>    * struct property_entry - "Built-in" device property representation.
>    * @name: Name of the property.
>    * @length: Length of data making up the value.
> - * @is_array: True when the property is an array.
> + * @is_inline: True when the property value is embedded in
> + *	&struct property_entry instance.
>    * @type: Type of the data in unions.
> - * @pointer: Pointer to the property (an array of items of the given type).
> - * @value: Value of the property (when it is a single item of the given type).
> + * @pointer: Pointer to the property when it is stored separately from
> + *	the &struct property_entry instance.
> + * @value: Value of the property when it is stored inline.
>    */
>   struct property_entry {
>   	const char *name;
>   	size_t length;
> -	bool is_array;
> +	bool is_inline;
>   	enum dev_prop_type type;
>   	union {
>   		const void *pointer;
> @@ -262,7 +264,6 @@ struct property_entry {
>   (struct property_entry) {						\
>   	.name = _name_,							\
>   	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
> -	.is_array = true,						\
>   	.type = DEV_PROP_##_Type_,					\
>   	{ .pointer = _val_ },						\
>   }
> @@ -293,6 +294,7 @@ struct property_entry {
>   (struct property_entry) {						\
>   	.name = _name_,							\
>   	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
> +	.is_inline = true,						\
>   	.type = DEV_PROP_##_Type_,					\
>   	{ .value = { ._elem_ = _val_ } },				\
>   }
> @@ -311,6 +313,7 @@ struct property_entry {
>   #define PROPERTY_ENTRY_BOOL(_name_)		\
>   (struct property_entry) {			\
>   	.name = _name_,				\
> +	.is_inline = true,			\
>   }
>   
>   struct property_entry *

Best regards
Andy Shevchenko Dec. 12, 2019, 11:28 a.m. UTC | #5
On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
> Dear All,
> 
> On 08.11.2019 05:22, Dmitry Torokhov wrote:
> > We do not need a special flag to know if we are dealing with an array,
> > as we can get that data from ratio between element length and the data
> > size, however we do need a flag to know whether the data is stored
> > directly inside property_entry or separately.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Today I've noticed that this patch got merged to linux-next as commit 
> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI 
> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs 
> which use DWC3 in host mode too). I get the following errors during boot:
> 
> dwc3 12000000.dwc3: failed to add properties to xHCI
> dwc3 12000000.dwc3: failed to initialize host
> dwc3: probe of 12000000.dwc3 failed with error -61
> 
> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
> 
> https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
> 
> (lack of 'ref' clk is not related nor fatal to the driver operation).
> 
> The code which fails after this patch is located in 
> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

Thank you for report.

I think we should not have that patch in the fist place... I used to have
a bad feeling about it and then forgot about it existence.
Rafael J. Wysocki Dec. 12, 2019, 4:41 p.m. UTC | #6
On Thu, Dec 12, 2019 at 12:28 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
> > Dear All,
> >
> > On 08.11.2019 05:22, Dmitry Torokhov wrote:
> > > We do not need a special flag to know if we are dealing with an array,
> > > as we can get that data from ratio between element length and the data
> > > size, however we do need a flag to know whether the data is stored
> > > directly inside property_entry or separately.
> > >
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > Today I've noticed that this patch got merged to linux-next as commit
> > e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI
> > driver operation on Samsung Exynos5 SoCs (and probably on other SoCs
> > which use DWC3 in host mode too). I get the following errors during boot:
> >
> > dwc3 12000000.dwc3: failed to add properties to xHCI
> > dwc3 12000000.dwc3: failed to initialize host
> > dwc3: probe of 12000000.dwc3 failed with error -61
> >
> > Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
> >
> > https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
> >
> > (lack of 'ref' clk is not related nor fatal to the driver operation).
> >
> > The code which fails after this patch is located in
> > drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
>
> Thank you for report.
>
> I think we should not have that patch in the fist place... I used to have
> a bad feeling about it and then forgot about it existence.

Well, I think you mean the [2/6].

The $subject one really shouldn't change functionality, we must have
missed something here.

Anyway, I'll drop this branch from the linux-next one for now.
Dmitry Torokhov Dec. 13, 2019, 1:24 a.m. UTC | #7
Hi Marek,

On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
> Dear All,
> 
> On 08.11.2019 05:22, Dmitry Torokhov wrote:
> > We do not need a special flag to know if we are dealing with an array,
> > as we can get that data from ratio between element length and the data
> > size, however we do need a flag to know whether the data is stored
> > directly inside property_entry or separately.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Today I've noticed that this patch got merged to linux-next as commit 
> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI 
> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs 
> which use DWC3 in host mode too). I get the following errors during boot:
> 
> dwc3 12000000.dwc3: failed to add properties to xHCI
> dwc3 12000000.dwc3: failed to initialize host
> dwc3: probe of 12000000.dwc3 failed with error -61
> 
> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
> 
> https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
> 
> (lack of 'ref' clk is not related nor fatal to the driver operation).
> 
> The code which fails after this patch is located in 
> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.

Does the following help? If, as I expect, it does, I'll submit it
formally.

---

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 5567ed2cddbec..fa252870c926f 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -88,10 +88,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
 
 	if (dwc->usb3_lpm_capable)
-		props[prop_idx++].name = "usb3-lpm-capable";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable");
 
 	if (dwc->usb2_lpm_disable)
-		props[prop_idx++].name = "usb2-lpm-disable";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
@@ -103,7 +103,7 @@ int dwc3_host_init(struct dwc3 *dwc)
 	 * This following flag tells XHCI to do just that.
 	 */
 	if (dwc->revision <= DWC3_REVISION_300A)
-		props[prop_idx++].name = "quirk-broken-port-ped";
+		props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");
 
 	if (prop_idx) {
 		ret = platform_device_add_properties(xhci, props);
Marek Szyprowski Dec. 13, 2019, 6:44 a.m. UTC | #8
Hi Dmitry,

On 13.12.2019 02:24, Dmitry Torokhov wrote:
> On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
>> On 08.11.2019 05:22, Dmitry Torokhov wrote:
>>> We do not need a special flag to know if we are dealing with an array,
>>> as we can get that data from ratio between element length and the data
>>> size, however we do need a flag to know whether the data is stored
>>> directly inside property_entry or separately.
>>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Today I've noticed that this patch got merged to linux-next as commit
>> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI
>> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs
>> which use DWC3 in host mode too). I get the following errors during boot:
>>
>> dwc3 12000000.dwc3: failed to add properties to xHCI
>> dwc3 12000000.dwc3: failed to initialize host
>> dwc3: probe of 12000000.dwc3 failed with error -61
>>
>> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
>>
>> https://protect2.fireeye.com/url?k=df93ba76-820d14ec-df923139-0cc47a336fae-f751d8b108bc5bdf&u=https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
>>
>> (lack of 'ref' clk is not related nor fatal to the driver operation).
>>
>> The code which fails after this patch is located in
>> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
> Does the following help? If, as I expect, it does, I'll submit it
> formally.

Yes, it fixes the issue. If possible, please add the following tags:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

to the final patch.

> ---
>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index 5567ed2cddbec..fa252870c926f 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -88,10 +88,10 @@ int dwc3_host_init(struct dwc3 *dwc)
>   	memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
>   
>   	if (dwc->usb3_lpm_capable)
> -		props[prop_idx++].name = "usb3-lpm-capable";
> +		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable");
>   
>   	if (dwc->usb2_lpm_disable)
> -		props[prop_idx++].name = "usb2-lpm-disable";
> +		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
>   
>   	/**
>   	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
> @@ -103,7 +103,7 @@ int dwc3_host_init(struct dwc3 *dwc)
>   	 * This following flag tells XHCI to do just that.
>   	 */
>   	if (dwc->revision <= DWC3_REVISION_300A)
> -		props[prop_idx++].name = "quirk-broken-port-ped";
> +		props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");
>   
>   	if (prop_idx) {
>   		ret = platform_device_add_properties(xhci, props);
>
>
Best regards
Marek Szyprowski Dec. 13, 2019, 6:47 a.m. UTC | #9
Hi Rafael,

On 12.12.2019 17:41, Rafael J. Wysocki wrote:
> On Thu, Dec 12, 2019 at 12:28 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
>>> On 08.11.2019 05:22, Dmitry Torokhov wrote:
>>>> We do not need a special flag to know if we are dealing with an array,
>>>> as we can get that data from ratio between element length and the data
>>>> size, however we do need a flag to know whether the data is stored
>>>> directly inside property_entry or separately.
>>>>
>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Today I've noticed that this patch got merged to linux-next as commit
>>> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI
>>> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs
>>> which use DWC3 in host mode too). I get the following errors during boot:
>>>
>>> dwc3 12000000.dwc3: failed to add properties to xHCI
>>> dwc3 12000000.dwc3: failed to initialize host
>>> dwc3: probe of 12000000.dwc3 failed with error -61
>>>
>>> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
>>>
>>> https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
>>>
>>> (lack of 'ref' clk is not related nor fatal to the driver operation).
>>>
>>> The code which fails after this patch is located in
>>> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
>> Thank you for report.
>>
>> I think we should not have that patch in the fist place... I used to have
>> a bad feeling about it and then forgot about it existence.
> Well, I think you mean the [2/6].
>
> The $subject one really shouldn't change functionality, we must have
> missed something here.

Nope, I was really talking about [1/6]. It looks that it revealed an 
issue in the DWC3 driver pointed by Dmitry.

> Anyway, I'll drop this branch from the linux-next one for now.

Best regards
Rafael J. Wysocki Dec. 13, 2019, 8:37 a.m. UTC | #10
On Fri, Dec 13, 2019 at 7:47 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Rafael,
>
> On 12.12.2019 17:41, Rafael J. Wysocki wrote:
> > On Thu, Dec 12, 2019 at 12:28 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >> On Thu, Dec 12, 2019 at 12:12:36PM +0100, Marek Szyprowski wrote:
> >>> On 08.11.2019 05:22, Dmitry Torokhov wrote:
> >>>> We do not need a special flag to know if we are dealing with an array,
> >>>> as we can get that data from ratio between element length and the data
> >>>> size, however we do need a flag to know whether the data is stored
> >>>> directly inside property_entry or separately.
> >>>>
> >>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>> Today I've noticed that this patch got merged to linux-next as commit
> >>> e6bff4665c595b5a4aff173848851ed49ac3bfad. Sadly it breaks DWC3/xHCI
> >>> driver operation on Samsung Exynos5 SoCs (and probably on other SoCs
> >>> which use DWC3 in host mode too). I get the following errors during boot:
> >>>
> >>> dwc3 12000000.dwc3: failed to add properties to xHCI
> >>> dwc3 12000000.dwc3: failed to initialize host
> >>> dwc3: probe of 12000000.dwc3 failed with error -61
> >>>
> >>> Here is a full kernel log from Exynos5250-based Snow Chromebook on KernelCI:
> >>>
> >>> https://storage.kernelci.org/next/master/next-20191212/arm/exynos_defconfig/gcc-8/lab-collabora/boot-exynos5250-snow.txt
> >>>
> >>> (lack of 'ref' clk is not related nor fatal to the driver operation).
> >>>
> >>> The code which fails after this patch is located in
> >>> drivers/usb/dwc3/host.c. Let me know if I can help more in locating the bug.
> >> Thank you for report.
> >>
> >> I think we should not have that patch in the fist place... I used to have
> >> a bad feeling about it and then forgot about it existence.
> > Well, I think you mean the [2/6].
> >
> > The $subject one really shouldn't change functionality, we must have
> > missed something here.
>
> Nope, I was really talking about [1/6]. It looks that it revealed an
> issue in the DWC3 driver pointed by Dmitry.

Right, but I was referring to the Andy's comment.

Cheers!
diff mbox series

Patch

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index d8d0dc0ca5acf..18a30fb3cc588 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -108,10 +108,7 @@  static const void *property_get_pointer(const struct property_entry *prop)
 	if (!prop->length)
 		return NULL;
 
-	if (prop->is_array)
-		return prop->pointer;
-
-	return &prop->value;
+	return prop->is_inline ? &prop->value : prop->pointer;
 }
 
 static const void *property_entry_find(const struct property_entry *props,
@@ -205,7 +202,7 @@  static void property_entry_free_data(const struct property_entry *p)
 	const char * const *src_str;
 	size_t i, nval;
 
-	if (p->is_array) {
+	if (!p->is_inline) {
 		if (p->type == DEV_PROP_STRING && p->pointer) {
 			src_str = p->pointer;
 			nval = p->length / sizeof(const char *);
@@ -250,7 +247,7 @@  static int property_entry_copy_data(struct property_entry *dst,
 	const void *pointer = property_get_pointer(src);
 	const void *new;
 
-	if (src->is_array) {
+	if (!src->is_inline) {
 		if (!src->length)
 			return -ENODATA;
 
@@ -264,15 +261,16 @@  static int property_entry_copy_data(struct property_entry *dst,
 				return -ENOMEM;
 		}
 
-		dst->is_array = true;
 		dst->pointer = new;
 	} else if (src->type == DEV_PROP_STRING) {
 		new = kstrdup(src->value.str, GFP_KERNEL);
 		if (!new && src->value.str)
 			return -ENOMEM;
 
+		dst->is_inline = true;
 		dst->value.str = new;
 	} else {
+		dst->is_inline = true;
 		dst->value = src->value;
 	}
 
diff --git a/include/linux/property.h b/include/linux/property.h
index 48335288c2a96..dad0ad11b55e2 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -227,15 +227,17 @@  static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
  * @length: Length of data making up the value.
- * @is_array: True when the property is an array.
+ * @is_inline: True when the property value is embedded in
+ *	&struct property_entry instance.
  * @type: Type of the data in unions.
- * @pointer: Pointer to the property (an array of items of the given type).
- * @value: Value of the property (when it is a single item of the given type).
+ * @pointer: Pointer to the property when it is stored separately from
+ *	the &struct property_entry instance.
+ * @value: Value of the property when it is stored inline.
  */
 struct property_entry {
 	const char *name;
 	size_t length;
-	bool is_array;
+	bool is_inline;
 	enum dev_prop_type type;
 	union {
 		const void *pointer;
@@ -262,7 +264,6 @@  struct property_entry {
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
-	.is_array = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = _val_ },						\
 }
@@ -293,6 +294,7 @@  struct property_entry {
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),		\
+	.is_inline = true,						\
 	.type = DEV_PROP_##_Type_,					\
 	{ .value = { ._elem_ = _val_ } },				\
 }
@@ -311,6 +313,7 @@  struct property_entry {
 #define PROPERTY_ENTRY_BOOL(_name_)		\
 (struct property_entry) {			\
 	.name = _name_,				\
+	.is_inline = true,			\
 }
 
 struct property_entry *