| 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: <20201103134421.7f940b25@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date: Tue, 3 Nov 2020 13:44:21 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: <min.li.xe@...esas.com>
Cc: <richardcochran@...il.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 net 1/1] ptp: idt82p33: add adjphase support
On Mon, 2 Nov 2020 10:27:28 -0500 min.li.xe@...esas.com wrote:
> From: Min Li <min.li.xe@...esas.com>
>
> Add idt82p33_adjwritephase() to support PHC write phase mode.
>
> Changes since v1:
> - Fix broken build on 32 bit machine due to 64 bit division.
>
> Signed-off-by: Min Li <min.li.xe@...esas.com>
> ---
> drivers/ptp/ptp_idt82p33.c | 317 ++++++++++++++++++++++++++++++++++-----------
> drivers/ptp/ptp_idt82p33.h | 3 +
> 2 files changed, 244 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
> index 179f6c4..ed1659e 100644
> --- a/drivers/ptp/ptp_idt82p33.c
> +++ b/drivers/ptp/ptp_idt82p33.c
> @@ -33,6 +33,9 @@ module_param(phase_snap_threshold, uint, 0);
> MODULE_PARM_DESC(phase_snap_threshold,
> "threshold (150000ns by default) below which adjtime would ignore");
>
> +static char *firmware;
> +module_param(firmware, charp, 0);
The commit message does not explain the need for FW. Please at least
mention it. Also please avoid the module parameter. Decide on a
standard name for the FW and use MODULE_FIRMWARE() to mark the
dependency.
> static void idt82p33_byte_array_to_timespec(struct timespec64 *ts,
> u8 buf[TOD_BYTE_COUNT])
> {
> @@ -77,15 +80,15 @@ static void idt82p33_timespec_to_byte_array(struct timespec64 const *ts,
> }
> }
>
> -static int idt82p33_xfer(struct idt82p33 *idt82p33,
> - unsigned char regaddr,
> - unsigned char *buf,
> - unsigned int count,
> - int write)
> +static int idt82p33_xfer_read(struct idt82p33 *idt82p33,
> + unsigned char regaddr,
> + unsigned char *buf,
> + unsigned int count)
> {
> struct i2c_client *client = idt82p33->client;
> struct i2c_msg msg[2];
> int cnt;
> + char *fmt = "i2c_transfer failed at %d in %s, at addr: %04X!\n";
Why are you moving the formats out? This is the first time I've seen
this done.
> msg[0].addr = client->addr;
> msg[0].flags = 0;
> @@ -93,13 +96,17 @@ static int idt82p33_xfer(struct idt82p33 *idt82p33,
> msg[0].buf = ®addr;
>
> msg[1].addr = client->addr;
> - msg[1].flags = write ? 0 : I2C_M_RD;
> + msg[1].flags = I2C_M_RD;
> msg[1].len = count;
> msg[1].buf = buf;
>
> cnt = i2c_transfer(client->adapter, msg, 2);
> if (cnt < 0) {
> - dev_err(&client->dev, "i2c_transfer returned %d\n", cnt);
> + dev_err(&client->dev,
> + fmt,
> + __LINE__,
> + __func__,
Do you really need those line references?
Please use meaningful messages instead.
> + (u8) regaddr);
Why the cast, it's already unsigned char.
> return cnt;
> } else if (cnt != 2) {
> dev_err(&client->dev,
> @@ -109,6 +116,37 @@ static int idt82p33_xfer(struct idt82p33 *idt82p33,
> return 0;
> }
>
> +static int idt82p33_xfer_write(struct idt82p33 *idt82p33,
> + u8 regaddr,
> + u8 *buf,
> + u16 count)
> +{
> + struct i2c_client *client = idt82p33->client;
> + /* we add 1 byte for device register */
> + u8 msg[IDT82P33_MAX_WRITE_COUNT + 1];
> + int cnt;
> + char *fmt = "i2c_master_send failed at %d in %s, at addr: %04X!\n";
> +
> + if (count > IDT82P33_MAX_WRITE_COUNT)
> + return -EINVAL;
> +
> + msg[0] = regaddr;
> + memcpy(&msg[1], buf, count);
> +
> + cnt = i2c_master_send(client, msg, count + 1);
> +
> + if (cnt < 0) {
Please don't put empty lines between a function call and its error checking.
Also looks like cnt should really be called 'err' or 'ret' here.
> + dev_err(&client->dev,
> + fmt,
> + __LINE__,
> + __func__,
> + regaddr);
> + return cnt;
> + }
> +
> + return 0;
> +}
> +
> static int idt82p33_page_offset(struct idt82p33 *idt82p33, unsigned char val)
> {
> int err;
> @@ -116,7 +154,7 @@ static int idt82p33_page_offset(struct idt82p33 *idt82p33, unsigned char val)
> if (idt82p33->page_offset == val)
> return 0;
>
> - err = idt82p33_xfer(idt82p33, PAGE_ADDR, &val, sizeof(val), 1);
> + err = idt82p33_xfer_write(idt82p33, PAGE_ADDR, &val, sizeof(val));
This refactoring for xfer() -> xfer_read()/xfer_write() should be a
separate patch, forming a series.
> if (err)
> dev_err(&idt82p33->client->dev,
> "failed to set page offset %d\n", val);
> @@ -314,16 +352,9 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
> * FCW = -------------
> * 168 * 2^4
> */
> - if (scaled_ppm < 0) {
> - neg_adj = 1;
> - scaled_ppm = -scaled_ppm;
> - }
>
> fcw = scaled_ppm * 244140625ULL;
> - fcw = div_u64(fcw, 2688);
> -
> - if (neg_adj)
> - fcw = -fcw;
> + fcw = div_s64(fcw, 2688);
Looks unrelated to adjphase support, make it a separate patch.
> for (i = 0; i < 5; i++) {
> buf[i] = fcw & 0xff;
> @@ -518,12 +554,10 @@ static int idt82p33_sync_tod(struct idt82p33_channel *channel, bool enable)
> u8 sync_cnfg;
> int err;
>
> - if (enable == channel->sync_tod_on) {
> - if (enable && sync_tod_timeout) {
> - mod_delayed_work(system_wq, &channel->sync_tod_work,
> - sync_tod_timeout * HZ);
> - }
> - return 0;
> + /* Turn it off after sync_tod_timeout seconds */
> + if (enable && sync_tod_timeout) {
> + ptp_schedule_worker(channel->ptp_clock,
> + sync_tod_timeout * HZ);
Parenthesis are not necessary around single line statements.
> }
>
> err = idt82p33_read(idt82p33, channel->dpll_sync_cnfg,
> +static int idt82p33_adjwritephase(struct ptp_clock_info *ptp, s32 offsetNs)
> +{
> + struct idt82p33_channel *channel =
> + container_of(ptp, struct idt82p33_channel, caps);
> + struct idt82p33 *idt82p33 = channel->idt82p33;
> + s64 offsetInFs;
> + s64 offsetRegVal;
> + u8 val[4] = {0};
Please order variable declaration lines longest to shortest.
> + int err;
> +
> + offsetInFs = (s64)(-offsetNs) * 1000000;
> +
> + if (offsetInFs > WRITE_PHASE_OFFSET_LIMIT)
> + offsetInFs = WRITE_PHASE_OFFSET_LIMIT;
> + else if (offsetInFs < -WRITE_PHASE_OFFSET_LIMIT)
> + offsetInFs = -WRITE_PHASE_OFFSET_LIMIT;
> +
> + /* Convert from phaseOffsetInFs to register value */
> + offsetRegVal = div_s64(offsetInFs * 1000, IDT_T0DPLL_PHASE_RESOL);
Powered by blists - more mailing lists