| 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
| ||
|
Message-ID: <20150723132251.GA3436@x1>
Date: Thu, 23 Jul 2015 14:22:51 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Bjorn Andersson <bjorn.andersson@...ymobile.com>
Cc: bjorn@...o.se, Andy Gross <agross@...eaurora.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 06/11] mfd: qcom-smd-rpm: Driver for the Qualcomm RPM
over SMD
On Mon, 13 Jul 2015, Bjorn Andersson wrote:
> On Tue 07 Jul 05:37 PDT 2015, Lee Jones wrote:
>
> > On Fri, 26 Jun 2015, bjorn@...o.se wrote:
> >
> > > From: Bjorn Andersson <bjorn.andersson@...ymobile.com>
> [..]
>
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>
> [..]
>
> > > +config MFD_QCOM_SMD_RPM
> > > + tristate "Qualcomm Resource Power Manager (RPM) over SMD"
> > > + depends on QCOM_SMD && OF
> > > + help
> > > + If you say yes to this option, support will be included for the
> > > + Resource Power Manager system found in the Qualcomm 8974 based
> > > + devices.
> > > +
> > > + This is required to access many regulators, clocks and bus
> > > + frequencies controlled by the RPM on these devices.
> > > +
> > > + Say M here if you want to include support for the Qualcomm RPM as a
> > > + module. This will build a module called "qcom-smd-rpm".
> >
> > I'm not exactly sure what makes this an MFD device.
> >
>
> It represents a piece of hardware (a micro-controller) that exposes
> control of a multitude of regulators and clocks in the Qualcomm
> platforms.
>
> It's basically just a successor of the qcom_rpm driver - same
> functionality but a new communication method is used.
My point still stands. Please investigate moving this (and the
qcom_rpm driver if it's the same) into either drivers/soc or
drivers/platform. The support in these two directories _seem_ to be
pretty similar.
> > > diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c
>
> [..]
>
> > > +
> > > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist"
> >
> > I don't like this at all.
> >
>
> Which part of it?
>
> It should probably be a static const char *, inlined in the function
> below. Would that be to your liking?
It would be better, but I never really see the point in initialising
variables with these types of messages. I'd get rid of the
superfluous chuff and just do:
memcmp(msg->message, "resource does not exist", 23);
> > > +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev,
> > > + const void *data,
> > > + size_t count)
> > > +{
> > > + const struct qcom_rpm_header *hdr = data;
> > > + const struct qcom_rpm_message *msg;
> > > + const size_t inv_res_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1;
> > > + struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev);
> > > + const u8 *buf = data + sizeof(struct qcom_rpm_header);
> > > + const u8 *end = buf + hdr->length;
> > > + int status = 0;
> > > +
> > > + if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST ||
> > > + hdr->length < sizeof(struct qcom_rpm_message)) {
> > > + dev_err(&qsdev->dev, "invalid request\n");
> > > + return 0;
> > > + }
> > > +
> > > + while (buf < end) {
> > > + msg = (struct qcom_rpm_message *)buf;
> > > + switch (msg->msg_type) {
> > > + case RPM_MSG_TYPE_MSG_ID:
> > > + break;
> > > + case RPM_MSG_TYPE_ERR:
> > > + if (msg->length == inv_res_len &&
> > > + !memcmp(msg->message,
> > > + RPM_ERR_INVALID_RESOURCE,
> > > + inv_res_len))
> >
> > strncpy(msg->message, "resource does not exist", 23);
> >
>
> No, I want to compare the content of msg->message with the string
Yes, I just noticed that.
> "resource does not exist" - as that's the only way to know what type of
> error we got.
>
> This is unfortunately how the protocol looks :/
What about either my memcmp suggestion above or this then:
strncmp(msg->message, "resource does not exist", 23);
> > > + status = -ENXIO;
> > > + else
> > > + status = -EIO;
> > > + break;
> > > + }
> > > +
> > > + buf = PTR_ALIGN(buf + 2 * sizeof(u32) + msg->length, 4);
> > > + }
> > > +
> > > + rpm->ack_status = status;
> > > + complete(&rpm->ack);
> > > + return 0;
> > > +}
> > > +
>
> [..]
>
> > > +
> > > +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> > > + .probe = qcom_smd_rpm_probe,
> > > + .remove = qcom_smd_rpm_remove,
> > > + .callback = qcom_smd_rpm_callback,
> > > + .driver = {
> > > + .name = "qcom_smd_rpm",
> > > + .owner = THIS_MODULE,
> >
> > Remove this line.
Still not 100% sure why you need your own 'special' driver struct. If
it's for the .callback, there are other ways to do this without having
to invent your own bus.
> The module_qcom_smd_driver does not initialize the .owner, but to follow
> the general direction of the kernel I can add that to the macro...
>
> > > + .of_match_table = qcom_smd_rpm_of_match,
> > > + },
> > > +};
> > > +
> > > +module_qcom_smd_driver(qcom_smd_rpm_driver);
> > > +
> > > +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@...ymobile.com>");
> > > +MODULE_DESCRIPTION("Qualcomm SMD backed RPM driver");
> > > +MODULE_LICENSE("GPL v2");
>
> Thanks,
> Bjorn
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists