linux-kernel - Re: [PATCH 5/5] reset: Add support for resets provided by SCMI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Jul 2019 11:34:24 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Philipp Zabel <p.zabel@...gutronix.de>
Cc:     linux-arm-kernel@...ts.infradead.org, Peng Fan <peng.fan@....com>,
        linux-kernel@...r.kernel.org,
        Bo Zhang <bozhang.zhang@...adcom.com>,
        Jim Quinlan <james.quinlan@...adcom.com>,
        Volodymyr Babchuk <volodymyr_babchuk@...m.com>,
        Gaku Inami <gaku.inami.xh@...esas.com>,
        aidapala@....qualcomm.com, pajay@....qualcomm.com,
        Etienne Carriere <etienne.carriere@...aro.org>,
        Souvik Chakravarty <Souvik.Chakravarty@....com>,
        wesleys@...inx.com, Felix Burton <fburton@...inx.com>,
        Saeed Nowshadi <saeed.nowshadi@...inx.com>
Subject: Re: [PATCH 5/5] reset: Add support for resets provided by SCMI

On Mon, Jul 29, 2019 at 11:59:15AM +0200, Philipp Zabel wrote:
> Hi Sudeep,
>
> On Fri, 2019-07-26 at 14:59 +0100, Sudeep Holla wrote:
> > On some ARM based systems, a separate Cortex-M based System Control
> > Processor(SCP) provides the overall power, clock, reset and system
> > control. System Control and Management Interface(SCMI) Message Protocol
> > is defined for the communication between the Application Cores(AP)
> > and the SCP.
> >
> > Adds support for the resets provided using SCMI protocol for performing
> > reset management of various devices present on the SoC. Various reset
> > functionalities are achieved by the means of different ARM SCMI device
> > operations provided by the ARM SCMI framework.
> >
> > Cc: Philipp Zabel <p.zabel@...gutronix.de>
> > Signed-off-by: Sudeep Holla <sudeep.holla@....com>
>
> thank you for the patch. I have a few suggestions below.
>

Thanks for reviewing so quickly, all points taken and fixed locally.
Will wait for some more time before posting v2.

[...]

> > +static int
> > +scmi_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
> > +	const struct scmi_handle *handle = data->handle;
>
> This could be shortened to to_scmi_handle(rcdev), since none of the
> other fields in scmi_reset_data are used by the reset_control_ops
> callbacks.
>

Makes sense, missed to see that.

[...]

> > +
> > +static int scmi_reset_probe(struct scmi_device *sdev)
> > +{
> > +	struct scmi_reset_data *data;
> > +	struct device *dev = &sdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	const struct scmi_handle *handle = sdev->handle;
> > +
> > +	if (!handle || !handle->reset_ops)
> > +		return -ENODEV;
> > +
> > +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->rcdev.ops = &scmi_reset_ops;
> > +	data->rcdev.owner = THIS_MODULE;
> > +	data->rcdev.of_node = np;
>
> This is missing rcdev.nr_resets. When nr_resets is kept at zero, the
> check in of_reset_simple_xlate will fail for any id.
>

I clearly missed to do git add for the above :(. I did find these
after testing.

> > +	data->dev = dev;
> > +
> > +	return devm_reset_controller_register(dev, &data->rcdev);
> > +}
> > +
> > +static const struct scmi_device_id scmi_id_table[] = {
> > +	{ SCMI_PROTOCOL_RESET },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> > +
> > +static struct scmi_driver scmi_reset_driver = {
> > +	.name = "scmi-reset",
> > +	.probe = scmi_reset_probe,
> > +	.id_table = scmi_id_table,
> > +};
> > +module_scmi_driver(scmi_reset_driver);
> > +
> > +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@....com>");
> > +MODULE_DESCRIPTION("ARM SCMI clock driver");
>
> s/clock/reset controller/
>

Stupid copy-paste :(

Thanks again.

--
Regards,
Sudeep