Re: [EXT] [bug report] net: aquantia: add basic ptp_clock callbacks

Re: [EXT] [bug report] net: aquantia: add basic ptp_clock callbacks

[Igor Russkikh]

DQpPbiAyOC4xMC4yMDE5IDE4OjI1LCBFZ29yIFBvbW96b3Ygd3JvdGU6DQo+IEhlbGxvIERhbiwN
Cj4gK0lnb3INCj4gDQo+IFRoYW5rIHlvdSBmb3IgcG9pbnRpbmcgdGhlIGlzc3VlLiBXZeKAmWxs
IGZpeGVkIHNvb24uDQoNCg0KPj4gIDEyMDcgICAgICAgICAgY2xvY2sgPSBwdHBfY2xvY2tfcmVn
aXN0ZXIoJmFxX3B0cC0+cHRwX2luZm8sICZhcV9uaWMtPm5kZXYtPmRldik7DQo+PiAgMTIwOCAg
ICAgICAgICBpZiAoIWNsb2NrIHx8IElTX0VSUihjbG9jaykpIHsNCj4+ICAxMjA5ICAgICAgICAg
ICAgICAgICAgbmV0ZGV2X2VycihhcV9uaWMtPm5kZXYsICJwdHBfY2xvY2tfcmVnaXN0ZXIgZmFp
bGVkXG4iKTsNCj4+ICAxMjEwICAgICAgICAgICAgICAgICAgZXJyID0gUFRSX0VSUihjbG9jayk7
DQo+PiAgMTIxMSAgICAgICAgICAgICAgICAgIGdvdG8gZXJyX2V4aXQ7DQo+Pg0KPj4gVGhpcyBp
cyBhIGZhbHNlIHBvc2l0aXZlIGluIFNtYXRjaCBidXQgdGhlIGNvZGUgaXMgc3RpbGwgcHJvYmxl
bWF0aWMuDQo+Pg0KPj4gVGhlIGlzc3VlIGlzIHRoYXQgcHRwX2Nsb2NrX3JlZ2lzdGVyKCkgcmV0
dXJucyBlcnJvciBwb2ludGVycyBpZiB0aGVyZQ0KPj4gaXMgYW4gZXJyb3IgYW5kIGl0IHJldHVy
bnMgTlVMTCBpZiB0aGUgY2xvY2sgaXMgZGlzYWJsZWQgaW4gdGhlIGNvbmZpZy4NCj4+IElmICJj
bG9jayIgaXMgTlVMTCB0aGVuIHRoaXMgY29kZSByZXR1cm5zIFBUUl9FUlIoTlVMTCkgd2hpY2gg
aXMNCj4+IHN1Y2Nlc3MgYnV0IHNvIHRoYXQncyBhIGJ1Zy4NCj4+DQo+PiBUaGUgcXVlc3Rpb24g
aXMsIGlzIGl0IHJlYWxseSByZWFsaXN0aWMgZm9yIHBlb3BsZSB0byBydW4gdGhpcyBoYXJkd2Fy
ZQ0KPj4gd2l0aG91dCBhIHB0cCBjbG9jaz8gIElmIHNvIHRoZW4gd2Ugc2hvdWxkIGFsbG93IGl0
IGluc3RlYWQgb2YgZXJyb3JpbmcNCj4+IG91dCBoZXJlIHdoZW4gY2xvY2sgaXMgTlVMTC4gIElm
IG5vdCB0aGVuIHdlIHNob3VsZCBlbmZvcmNlIHRoYXQgaW4gdGhlDQo+PiBLY29uZmlnIGluc3Rl
YWQgb2Ygd2FpdGluZyB1bnRpbCBydW50aW1lLg0KDQpIaSBEYW4sIEknZCBzYXkgdGhhdHMgYSBm
YWxzZSBwb3NpdGl2ZS4NClRoZXJlIGV4aXN0IEhXIGNvbmZpZ3VyYXRpb24gd2hlcmUgUFRQIGlz
IGRpc2FibGVkIG9yIG5vdCBhdmFpbGFibGUuDQoNClBUUl9FUlIoTlVMTCkgaXMgMCwgc28gZXZl
bnR1YWxseSBkcml2ZXIgbm93IGZ1bmN0aW9ucyBjb3JyZWN0bHksIGFsbG93aW5nIHRvDQpwcm9j
ZWVkIGJ1dCBtYXJraW5nIHB0cCBmdW5jdGlvbmFsaXR5IGFzIGRpc2FibGVkLg0KDQpJIGFncmVl
IHRoYXQncyBhIGJpdCBjb3VudGVyaW50dWl0aXZlIGJ1dCBjb3JyZWN0Lg0KDQpSZWdhcmRzLA0K
ICBJZ29yDQo

Re: [EXT] [bug report] net: aquantia: add basic ptp_clock callbacks

[Dan Carpenter]

On Tue, Oct 29, 2019 at 09:14:33AM +0000, Igor Russkikh wrote:
> 
> On 28.10.2019 18:25, Egor Pomozov wrote:
> > Hello Dan,
> > +Igor
> > 
> > Thank you for pointing the issue. We’ll fixed soon.
> 
> 
> >>  1207          clock = ptp_clock_register(&aq_ptp->ptp_info, &aq_nic->ndev->dev);
> >>  1208          if (!clock || IS_ERR(clock)) {
> >>  1209                  netdev_err(aq_nic->ndev, "ptp_clock_register failed\n");
> >>  1210                  err = PTR_ERR(clock);
> >>  1211                  goto err_exit;
> >>
> >> This is a false positive in Smatch but the code is still problematic.
> >>
> >> The issue is that ptp_clock_register() returns error pointers if there
> >> is an error and it returns NULL if the clock is disabled in the config.
> >> If "clock" is NULL then this code returns PTR_ERR(NULL) which is
> >> success but so that's a bug.
> >>
> >> The question is, is it really realistic for people to run this hardware
> >> without a ptp clock?  If so then we should allow it instead of erroring
> >> out here when clock is NULL.  If not then we should enforce that in the
> >> Kconfig instead of waiting until runtime.
> 
> Hi Dan, I'd say thats a false positive.
> There exist HW configuration where PTP is disabled or not available.
> 
> PTR_ERR(NULL) is 0, so eventually driver now functions correctly, allowing to
> proceed but marking ptp functionality as disabled.
> 
> I agree that's a bit counterintuitive but correct.

I have looked at it again and the "!clock" check should be removed
because it is dead code.  It's not possible for it to be NULL there
because aq_ptp_init() is a dummy function if the PTP clock is not
enabled.

regards,
dan carpenter

Re: [EXT] [bug report] net: aquantia: add basic ptp_clock callbacks

[Igor Russkikh]

DQo+PiBIaSBEYW4sIEknZCBzYXkgdGhhdHMgYSBmYWxzZSBwb3NpdGl2ZS4NCj4+IFRoZXJlIGV4
aXN0IEhXIGNvbmZpZ3VyYXRpb24gd2hlcmUgUFRQIGlzIGRpc2FibGVkIG9yIG5vdCBhdmFpbGFi
bGUuDQo+Pg0KPj4gUFRSX0VSUihOVUxMKSBpcyAwLCBzbyBldmVudHVhbGx5IGRyaXZlciBub3cg
ZnVuY3Rpb25zIGNvcnJlY3RseSwgYWxsb3dpbmcgdG8NCj4+IHByb2NlZWQgYnV0IG1hcmtpbmcg
cHRwIGZ1bmN0aW9uYWxpdHkgYXMgZGlzYWJsZWQuDQo+Pg0KPj4gSSBhZ3JlZSB0aGF0J3MgYSBi
aXQgY291bnRlcmludHVpdGl2ZSBidXQgY29ycmVjdC4NCj4gDQo+IEkgaGF2ZSBsb29rZWQgYXQg
aXQgYWdhaW4gYW5kIHRoZSAiIWNsb2NrIiBjaGVjayBzaG91bGQgYmUgcmVtb3ZlZA0KPiBiZWNh
dXNlIGl0IGlzIGRlYWQgY29kZS4gIEl0J3Mgbm90IHBvc3NpYmxlIGZvciBpdCB0byBiZSBOVUxM
IHRoZXJlDQo+IGJlY2F1c2UgYXFfcHRwX2luaXQoKSBpcyBhIGR1bW15IGZ1bmN0aW9uIGlmIHRo
ZSBQVFAgY2xvY2sgaXMgbm90DQo+IGVuYWJsZWQuDQoNCkhtLCBzZWVtcyB5b3UgYXJlIHJpZ2h0
LiBJJ3ZlIGFkZGVkIGV4Y2x1c2lvbiBjb2RlIHdpdGggYSBsYXRlciBjb21taXQNCmFuZCB5ZXMs
IHRoaXMgbWVhbnMgTlVMTCB3aWxsIG5ldmVyIGJlIGEgcmV0dmFsIGhlcmUuDQoNClRoYW5rcyBh
Z2FpbiwgSSdsbCBjbGVhbiBpdCB1cC4NCg0KLS0gDQpSZWdhcmRzLA0KICBJZ29yDQo