linux-kernel - Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

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:   Fri, 27 Sep 2019 14:25:18 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     vincent.cheng.xh@...esas.com
Cc:     robh+dt@...nel.org, mark.rutland@....com, richardcochran@...il.com,
        devicetree@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

> +static s32 idtcm_xfer(struct idtcm *idtcm,
> +		      u8 regaddr,
> +		      u8 *buf,
> +		      u16 count,
> +		      bool write)
> +{
> +	struct i2c_client *client = idtcm->client;
> +	struct i2c_msg msg[2];
> +	s32 cnt;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = 0;
> +	msg[0].len = 1;
> +	msg[0].buf = &regaddr;
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = write ? 0 : 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);
> +		return cnt;
> +	} else if (cnt != 2) {
> +		dev_err(&client->dev,
> +			"i2c_transfer sent only %d of %d messages\n", cnt, 2);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static s32 idtcm_page_offset(struct idtcm *idtcm, u8 val)
> +{
> +	u8 buf[4];
> +	s32 err;

Hi Vincent

All your functions return s32, rather than the usual int. err is an
s32.  i2c_transfer() will return an int, which you then assign to an
s32.  I've no idea, but maybe the static code checkers like smatch
will complain about this, especially on 64 bit systems? I suspect on
64 bit machines, the compiler will be generating worse code, masking
registers? Maybe use int, not s32?

> +static s32 set_pll_output_mask(struct idtcm *idtcm, u16 addr, u8 val)
> +{
> +	s32 err = 0;
> +
> +	switch (addr) {
> +	case OUTPUT_MASK_PLL0_ADDR:
> +		SET_U16_LSB(idtcm->channel[0].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL0_ADDR + 1:
> +		SET_U16_MSB(idtcm->channel[0].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL1_ADDR:
> +		SET_U16_LSB(idtcm->channel[1].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL1_ADDR + 1:
> +		SET_U16_MSB(idtcm->channel[1].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL2_ADDR:
> +		SET_U16_LSB(idtcm->channel[2].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL2_ADDR + 1:
> +		SET_U16_MSB(idtcm->channel[2].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL3_ADDR:
> +		SET_U16_LSB(idtcm->channel[3].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL3_ADDR + 1:
> +		SET_U16_MSB(idtcm->channel[3].output_mask, val);
> +		break;
> +	default:
> +		err = -1;

EINVAL?

> +		break;
> +	}
> +
> +	return err;
> +}

> +static void set_default_function_pointers(struct idtcm *idtcm)
> +{
> +	idtcm->_idtcm_gettime = _idtcm_gettime;
> +	idtcm->_idtcm_settime = _idtcm_settime;
> +	idtcm->_idtcm_rdwr = idtcm_rdwr;
> +	idtcm->_sync_pll_output = sync_pll_output;
> +}

Why does this indirection? Are the SPI versions of the silicon?

    Andrew