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]

Message-ID: <20190927141215.GA24424@renesas.com>
Date:   Fri, 27 Sep 2019 14:12:30 +0000
From:   Vincent Cheng <vincent.cheng.xh@...esas.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

On Fri, Sep 27, 2019 at 08:25:18AM EDT, Andrew Lunn wrote:
>> +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

Hi Andrew,

Thank-you for looking at the patch.

>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?

Oops.  You are correct, I messed up when trying to standardize
on linux types.h.  I will go through the code to ensure int is used
for error codes and return values.

>> +	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?

Yes, will replace with -EINVAL.  Thanks.

>> +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?

The indirection is to enable us to replace those functions in
our unit tests with mocked functions.

I read somewhere that I should leave a week between sending a
revised patch series.  Is this a good rule to follow?

Regards,
Vincent