| Message ID | 20240410165222.5192-1-prosunofficial@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: mux: replace of_node_put() with __free [linux-next] | expand |
On Wed, 10 Apr 2024 at 19:52, R SUNDAR <prosunofficial@gmail.com> wrote: > > use the new cleanup magic to replace of_node_put() with > __free(device_node) marking to auto release and to simplify the error > paths. > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: R SUNDAR <prosunofficial@gmail.com> > --- > drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++----------------- > 1 file changed, 8 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c > index b17826713753..72ec9ef3ac58 100644 > --- a/drivers/usb/typec/mux/nb7vpq904m.c > +++ b/drivers/usb/typec/mux/nb7vpq904m.c > @@ -315,29 +315,27 @@ static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = { > > static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) > { > - struct device_node *ep; > u32 data_lanes[4]; > int ret, i, j; > - > - ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0); > + struct device_node *ep __free(device_node) = > + of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0); Please stick this to the top of the function. Otherwise LGTM. > > if (ep) { > ret = of_property_count_u32_elems(ep, "data-lanes"); > if (ret == -EINVAL) > /* Property isn't here, consider default mapping */ > - goto out_done; > + return 0; > if (ret < 0) > - goto out_error; > + return ret; > > if (ret != DATA_LANES_COUNT) { > dev_err(&nb7->client->dev, "expected 4 data lanes\n"); > - ret = -EINVAL; > - goto out_error; > + return -EINVAL; > } > > ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT); > if (ret) > - goto out_error; > + return ret; > > for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) { > for (j = 0; j < DATA_LANES_COUNT; j++) { > @@ -358,18 +356,11 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) > break; > default: > dev_err(&nb7->client->dev, "invalid data lanes mapping\n"); > - ret = -EINVAL; > - goto out_error; > + return -EINVAL; > } > } > > -out_done: > - ret = 0; > - > -out_error: > - of_node_put(ep); > - > - return ret; > + return 0; > } > > static int nb7vpq904m_probe(struct i2c_client *client) > -- > 2.34.1 >
On Wed, Apr 10, 2024 at 10:22:22PM +0530, R SUNDAR wrote: > use the new cleanup magic to replace of_node_put() with > __free(device_node) marking to auto release and to simplify the error > paths. > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: R SUNDAR <prosunofficial@gmail.com> All CAPS for your name? Is that how it is written on documents (for some countries this is normal, but usually not, so I have to ask.) > --- > drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++----------------- > 1 file changed, 8 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c > index b17826713753..72ec9ef3ac58 100644 > --- a/drivers/usb/typec/mux/nb7vpq904m.c > +++ b/drivers/usb/typec/mux/nb7vpq904m.c > @@ -315,29 +315,27 @@ static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = { > > static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) > { > - struct device_node *ep; > u32 data_lanes[4]; > int ret, i, j; > - > - ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0); > + struct device_node *ep __free(device_node) = This use of __free() for device_node would be the first one in the tree (outside of a macro in a .h file.) Is this really what the OF maintainer wants to have happen here for this type of pattern? And if so, why this random USB driver being the first one? Have you tested this? > + of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0); > > if (ep) { Nit, this function should be rewritten to not work like this, the "common" path should not be indented, but only the exception (i.e. bail if ep is not allocated properly.) thanks, greg k-h
Hi greg k-h, Thanks for comments provided. On 11/04/24 17:47, Greg KH wrote: > On Wed, Apr 10, 2024 at 10:22:22PM +0530, R SUNDAR wrote: >> use the new cleanup magic to replace of_node_put() with >> __free(device_node) marking to auto release and to simplify the error >> paths. >> >> Suggested-by: Julia Lawall <julia.lawall@inria.fr> >> Signed-off-by: R SUNDAR <prosunofficial@gmail.com> > > All CAPS for your name? Is that how it is written on documents (for > some countries this is normal, but usually not, so I have to ask.) In documents it was like that.I also thought to change to small letters in git user name. will change to small letter. >> - >> - ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0); >> + struct device_node *ep __free(device_node) = > > And if so, why this random USB driver being the first one? Have you > tested this? I haven't tested it.I picked this usb driver to get cleanup module changes with __free. I believe since of_node_put() is scope based here,we can use cleanup to remove the goto label uses in case of error. > > >> + of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0); >> >> if (ep) { > > the "common" path should not be indented, but only the exception (i.e. bail > if ep is not allocated properly.) I Tried in same line initialization, it exceeds column limit. so indented in next line. > > thanks, > > greg k-h Regards, Sundar
diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c index b17826713753..72ec9ef3ac58 100644 --- a/drivers/usb/typec/mux/nb7vpq904m.c +++ b/drivers/usb/typec/mux/nb7vpq904m.c @@ -315,29 +315,27 @@ static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = { static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) { - struct device_node *ep; u32 data_lanes[4]; int ret, i, j; - - ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0); + struct device_node *ep __free(device_node) = + of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0); if (ep) { ret = of_property_count_u32_elems(ep, "data-lanes"); if (ret == -EINVAL) /* Property isn't here, consider default mapping */ - goto out_done; + return 0; if (ret < 0) - goto out_error; + return ret; if (ret != DATA_LANES_COUNT) { dev_err(&nb7->client->dev, "expected 4 data lanes\n"); - ret = -EINVAL; - goto out_error; + return -EINVAL; } ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT); if (ret) - goto out_error; + return ret; for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) { for (j = 0; j < DATA_LANES_COUNT; j++) { @@ -358,18 +356,11 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) break; default: dev_err(&nb7->client->dev, "invalid data lanes mapping\n"); - ret = -EINVAL; - goto out_error; + return -EINVAL; } } -out_done: - ret = 0; - -out_error: - of_node_put(ep); - - return ret; + return 0; } static int nb7vpq904m_probe(struct i2c_client *client)
use the new cleanup magic to replace of_node_put() with __free(device_node) marking to auto release and to simplify the error paths. Suggested-by: Julia Lawall <julia.lawall@inria.fr> Signed-off-by: R SUNDAR <prosunofficial@gmail.com> --- drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)