Messages in this thread | | | Date | Tue, 2 Apr 2024 17:05:19 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support |
| |
On Tue, Apr 02, 2024 at 01:27:19PM +0000, Peng Fan wrote: > > On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
..
> > > +#include <linux/module.h> > > > +#include <linux/scmi_protocol.h> > > > +#include <linux/slab.h> > > > > Please, follow IWYU principle, a lot of headers are missed. > > ok. I will try to figure out. BTW, is there an easy way to filter > out what is missed?
For you is much easier than to me as I'm not familiar with the code. Basically you should know what you wrote :-)
But if you are asking about tooling, we would appreciate when somebody comes with a such.
> > > +#include "common.h" > > > +#include "protocols.h"
..
> > > + ret = scmi_pinctrl_get_pin_info(ph, selector, > > > + &pi->pins[selector]); > > > > It's netter as a single line. > > I try to follow 80 max chars per SCMI coding style. If Sudeep and Cristian > is ok, I could use a single line.
It's minor, but even before relaxation of 80 limit it was and is mentioned in the documentation that you may go over if it increases readability.
> > > + if (ret) > > > + return ret; > > > + }
..
> > > +static int scmi_pinctrl_protocol_init(const struct > > > +scmi_protocol_handle *ph) { > > > + int ret; > > > + u32 version; > > > + struct scmi_pinctrl_info *pinfo; > > > + > > > + ret = ph->xops->version_get(ph, &version); > > > + if (ret) > > > + return ret; > > > + > > > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n", > > > + PROTOCOL_REV_MAJOR(version), > > PROTOCOL_REV_MINOR(version)); > > > + > > > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL); > > > + if (!pinfo) > > > + return -ENOMEM; > > > + > > > + ret = scmi_pinctrl_attributes_get(ph, pinfo); > > > + if (ret) > > > + return ret; > > > + > > > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins, > > > + sizeof(*pinfo->pins), GFP_KERNEL); > > > + if (!pinfo->pins) > > > + return -ENOMEM; > > > + > > > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups, > > > + sizeof(*pinfo->groups), GFP_KERNEL); > > > + if (!pinfo->groups) > > > + return -ENOMEM; > > > + > > > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions, > > > + sizeof(*pinfo->functions), > > GFP_KERNEL); > > > + if (!pinfo->functions) > > > + return -ENOMEM; > > > + > > > + pinfo->version = version; > > > + > > > + return ph->set_priv(ph, pinfo, version); > > > > Same comments as per previous version. devm_ here is simply wrong. > > It breaks the order of freeing resources. > > > > I.o.w. I see *no guarantee* that these init-deinit functions will be properly > > called from the respective probe-remove. Moreover the latter one may also > > have its own devm allocations (which are rightfully placed) and you get > > completely out of control the resource management. > > I see an old thread. > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t > > The free in deinit is not to free the ones alloced in init, it is to free the ones > alloced such as in scmi_pinctrl_get_function_info
Even messier than I thought. For bare minimum these two should be documented and renamed accordingly that no-one will think that deinit is a counter part of init.
-- With Best Regards, Andy Shevchenko
| |