| 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: <ZBBG2xRhLOIPMD0+@nanopsycho>
Date: Tue, 14 Mar 2023 11:05:15 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Vadim Fedorenko <vadfed@...a.com>
Cc: Jakub Kicinski <kuba@...nel.org>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>, poros@...hat.com,
mschmidt@...hat.com, netdev@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH RFC v6 6/6] ptp_ocp: implement DPLL ops
Sun, Mar 12, 2023 at 03:28:07AM CET, vadfed@...a.com wrote:
>Implement basic DPLL operations in ptp_ocp driver as the
>simplest example of using new subsystem.
>
>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>---
> drivers/ptp/Kconfig | 1 +
> drivers/ptp/ptp_ocp.c | 209 ++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 200 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
>index fe4971b65c64..8c4cfabc1bfa 100644
>--- a/drivers/ptp/Kconfig
>+++ b/drivers/ptp/Kconfig
>@@ -177,6 +177,7 @@ config PTP_1588_CLOCK_OCP
> depends on COMMON_CLK
> select NET_DEVLINK
> select CRC16
>+ select DPLL
> help
> This driver adds support for an OpenCompute time card.
>
>diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>index 4bbaccd543ad..02c95e724ec8 100644
>--- a/drivers/ptp/ptp_ocp.c
>+++ b/drivers/ptp/ptp_ocp.c
>@@ -23,6 +23,8 @@
> #include <linux/mtd/mtd.h>
> #include <linux/nvmem-consumer.h>
> #include <linux/crc16.h>
>+#include <linux/dpll.h>
>+#include <uapi/linux/dpll.h>
Don't include UAPI directly. I'm pretty sure I had this comment earlier.
>
> #define PCI_VENDOR_ID_FACEBOOK 0x1d9b
> #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400
>@@ -267,6 +269,7 @@ struct ptp_ocp_sma_connector {
> bool fixed_dir;
> bool disabled;
> u8 default_fcn;
>+ struct dpll_pin *dpll_pin;
> };
>
> struct ocp_attr_group {
>@@ -353,6 +356,7 @@ struct ptp_ocp {
> struct ptp_ocp_signal signal[4];
> struct ptp_ocp_sma_connector sma[4];
It is quite customary to use defines for const numbers like this. Why
don't you do that in ptp_ocp?
> const struct ocp_sma_op *sma_op;
>+ struct dpll_device *dpll;
> };
>
> #define OCP_REQ_TIMESTAMP BIT(0)
>@@ -2689,16 +2693,9 @@ sma4_show(struct device *dev, struct device_attribute *attr, char *buf)
> }
>
> static int
>-ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
>+ptp_ocp_sma_store_val(struct ptp_ocp *bp, int val, enum ptp_ocp_sma_mode mode, int sma_nr)
> {
> struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1];
>- enum ptp_ocp_sma_mode mode;
>- int val;
>-
>- mode = sma->mode;
>- val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode);
>- if (val < 0)
>- return val;
>
> if (sma->fixed_dir && (mode != sma->mode || val & SMA_DISABLE))
> return -EOPNOTSUPP;
>@@ -2733,6 +2730,21 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
> return val;
> }
>
>+static int
>+ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
>+{
>+ struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1];
>+ enum ptp_ocp_sma_mode mode;
>+ int val;
>+
>+ mode = sma->mode;
>+ val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode);
>+ if (val < 0)
>+ return val;
>+
>+ return ptp_ocp_sma_store_val(bp, val, mode, sma_nr);
>+}
>+
> static ssize_t
> sma1_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
>@@ -4171,12 +4183,151 @@ ptp_ocp_detach(struct ptp_ocp *bp)
> device_unregister(&bp->dev);
> }
>
>+static int ptp_ocp_dpll_pin_to_sma(const struct ptp_ocp *bp, const struct dpll_pin *pin)
>+{
>+ int i;
>+
>+ for (i = 0; i < 4; i++) {
>+ if (bp->sma[i].dpll_pin == pin)
>+ return i;
Just pass &bp->sma[i] as a priv to dpll_pin_register().
and get that pointer directly in pin ops. No need for lookup and use of
sma_nr at all.
>+ }
>+ return -1;
>+}
>+
>+static int ptp_ocp_dpll_lock_status_get(const struct dpll_device *dpll,
>+ enum dpll_lock_status *status,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
No need to cast (void *), remove it.
>+ int sync;
>+
>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>+ *status = sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED;
>+
>+ return 0;
>+}
>+
>+static int ptp_ocp_dpll_source_idx_get(const struct dpll_device *dpll,
>+ u32 *idx, struct netlink_ext_ack *extack)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+
>+ if (bp->pps_select) {
>+ *idx = ioread32(&bp->pps_select->gpio1);
>+ return 0;
>+ }
>+ return -EINVAL;
>+}
>+
>+static int ptp_ocp_dpll_mode_get(const struct dpll_device *dpll,
>+ u32 *mode, struct netlink_ext_ack *extack)
>+{
>+ *mode = DPLL_MODE_AUTOMATIC;
>+
No need for empty line, I believe.
>+ return 0;
>+}
>+
>+static bool ptp_ocp_dpll_mode_supported(const struct dpll_device *dpll,
>+ const enum dpll_mode mode,
>+ struct netlink_ext_ack *extack)
>+{
>+ if (mode == DPLL_MODE_AUTOMATIC)
>+ return true;
>+
>+ return false;
Just:
return mode == DPLL_MODE_AUTOMATIC;
>+}
>+
>+static int ptp_ocp_dpll_direction_get(const struct dpll_pin *pin,
>+ const struct dpll_device *dpll,
>+ enum dpll_pin_direction *direction,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+ int sma_nr = ptp_ocp_dpll_pin_to_sma(bp, pin);
>+
>+ if (sma_nr < 0)
>+ return -EINVAL;
>+
>+ *direction = bp->sma[sma_nr].mode == SMA_MODE_IN ? DPLL_PIN_DIRECTION_SOURCE :
>+ DPLL_PIN_DIRECTION_OUTPUT;
>+ return 0;
>+}
>+
>+static int ptp_ocp_dpll_direction_set(const struct dpll_pin *pin,
>+ const struct dpll_device *dpll,
>+ enum dpll_pin_direction direction,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+ int sma_nr = ptp_ocp_dpll_pin_to_sma(bp, pin);
>+ enum ptp_ocp_sma_mode mode;
>+
>+ if (sma_nr < 0)
>+ return -EINVAL;
>+
>+ mode = direction == DPLL_PIN_DIRECTION_SOURCE ? SMA_MODE_IN : SMA_MODE_OUT;
>+ return ptp_ocp_sma_store_val(bp, 0, mode, sma_nr);
>+}
>+
>+static int ptp_ocp_dpll_frequency_set(const struct dpll_pin *pin,
>+ const struct dpll_device *dpll,
>+ const u32 frequency,
Why you need const for u32?
>+ struct netlink_ext_ack *extack)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+ int sma_nr = ptp_ocp_dpll_pin_to_sma(bp, pin);
>+ int val = frequency == 10000000 ? 0 : 1;
This is weird. I believe you should handle unsupported frequencies
gracefully.
>+
>+ if (sma_nr < 0)
>+ return -EINVAL;
>+
>+
Avoid double empty lines.
>+ return ptp_ocp_sma_store_val(bp, val, bp->sma[sma_nr].mode, sma_nr);
>+}
>+
>+static int ptp_ocp_dpll_frequency_get(const struct dpll_pin *pin,
>+ const struct dpll_device *dpll,
>+ u32 *frequency,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+ int sma_nr = ptp_ocp_dpll_pin_to_sma(bp, pin);
>+ u32 val;
>+
>+ if (sma_nr < 0)
>+ return -EINVAL;
>+
>+ val = bp->sma_op->get(bp, sma_nr);
>+ if (!val)
>+ *frequency = 1000000;
>+ else
>+ *frequency = 0;
I don't follow. How the frequency could be 0? Does that mean that the
pin is disabled? If yes, you should rather implement
.state_on_dpll_get
.state_on_dpll_set
and leave the frequency constant.
>+ return 0;
>+}
>+
>+static struct dpll_device_ops dpll_ops = {
const
>+ .lock_status_get = ptp_ocp_dpll_lock_status_get,
>+ .source_pin_idx_get = ptp_ocp_dpll_source_idx_get,
Should be called "source_pin_get" and return (struct dpll_pin *)
On the driver api level, no reason to pass indexes. Work with objects.
>+ .mode_get = ptp_ocp_dpll_mode_get,
>+ .mode_supported = ptp_ocp_dpll_mode_supported,
>+};
>+
>+static struct dpll_pin_ops dpll_pins_ops = {
const
>+ .frequency_get = ptp_ocp_dpll_frequency_get,
>+ .frequency_set = ptp_ocp_dpll_frequency_set,
>+ .direction_get = ptp_ocp_dpll_direction_get,
>+ .direction_set = ptp_ocp_dpll_direction_set,
>+};
>+
> static int
> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
>+ struct dpll_pin_properties prop;
> struct devlink *devlink;
>+ char sma[4] = "SMA0";
Won't fit. Just use:
char *sma = "SMA0"
to be safe
> struct ptp_ocp *bp;
>- int err;
>+ int err, i;
>+ u64 clkid;
>
> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev);
> if (!devlink) {
>@@ -4226,8 +4377,44 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
> ptp_ocp_info(bp);
> devlink_register(devlink);
>- return 0;
>
>+ clkid = pci_get_dsn(pdev);
>+ bp->dpll = dpll_device_get(clkid, 0, THIS_MODULE);
>+ if (!bp->dpll) {
>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n");
>+ goto out;
>+ }
>+
>+ err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp, &pdev->dev);
>+ if (err)
>+ goto out;
>+
>+ prop.description = &sma[0];
>+ prop.freq_supported = DPLL_PIN_FREQ_SUPP_MAX;
>+ prop.type = DPLL_PIN_TYPE_EXT;
>+ prop.any_freq_max = 10000000;
>+ prop.any_freq_min = 0;
>+ prop.capabilities = DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE;
>+
>+ for (i = 0; i < 4; i++) {
>+ sma[3] = 0x31 + i;
Ugh. Just use the static const array as I suggested in the dpll patch
reply. Helps you avoid this sort of "magic".
>+ bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE, &prop);
>+ if (IS_ERR_OR_NULL(bp->sma[i].dpll_pin)) {
How it could be NULL?
>+ bp->sma[i].dpll_pin = NULL;
This would not be needed if the error path iterates over
indexes which were successul.
>+ goto out_dpll;
>+ }
>+ err = dpll_pin_register(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, bp, NULL);
>+ if (err)
>+ goto out_dpll;
>+ }
>+
>+ return 0;
>+out_dpll:
>+ for (i = 0; i < 4; i++) {
>+ if (bp->sma[i].dpll_pin)
You don't do unregister of pin here.
>+ dpll_pin_put(bp->sma[i].dpll_pin);
>+ }
>+ dpll_device_put(bp->dpll);
> out:
> ptp_ocp_detach(bp);
> out_disable:
>@@ -4243,6 +4430,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
> struct ptp_ocp *bp = pci_get_drvdata(pdev);
> struct devlink *devlink = priv_to_devlink(bp);
>
>+ dpll_device_unregister(bp->dpll);
>+ dpll_device_put(bp->dpll);
> devlink_unregister(devlink);
> ptp_ocp_detach(bp);
> pci_disable_device(pdev);
>--
>2.34.1
>
Powered by blists - more mailing lists