From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752122AbdEUHtE (ORCPT ); Sun, 21 May 2017 03:49:04 -0400 Received: from mga05.intel.com ([192.55.52.43]:13210 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751967AbdEUHtC (ORCPT ); Sun, 21 May 2017 03:49:02 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,373,1491289200"; d="scan'208";a="1150833829" Date: Sun, 21 May 2017 10:48:19 +0300 From: Mika Westerberg To: Lukas Wunner Cc: Greg Kroah-Hartman , Andreas Noever , Michael Jamet , Yehezkel Bernat , Amir Levy , Andy Lutomirski , Mario.Limonciello@dell.com, Jared.Dominguez@dell.com, Andy Shevchenko , linux-kernel@vger.kernel.org Subject: Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Message-ID: <20170521074819.GL8541@lahna.fi.intel.com> References: <20170518143914.60902-1-mika.westerberg@linux.intel.com> <20170518143914.60902-11-mika.westerberg@linux.intel.com> <20170519100710.GA15926@wunner.de> <20170519102836.GE8541@lahna.fi.intel.com> <20170521053114.GA3003@wunner.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170521053114.GA3003@wunner.de> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, May 21, 2017 at 07:31:14AM +0200, Lukas Wunner wrote: > On Fri, May 19, 2017 at 01:28:36PM +0300, Mika Westerberg wrote: > > On Fri, May 19, 2017 at 12:07:10PM +0200, Lukas Wunner wrote: > > > Apple uses 0x30 to store a > > > serial number. Is this attribute number assigned by Intel to Apple > > > or is it reserved for vendor use or did they arbitrarily choose it? > > > > It is part of the DROM specification. The 0x30 - 0x3e are vendor > > specific entries. > > Ah, so I have to qualify the vendor number with Apple's ID before I know > that it's a serial number. Thanks. Yes, something like that works. > > > If there can be many attributes, should they be stored in a list > > > rather than adding a char* pointer for each one to struct tb_switch? > > > The latter doesn't scale. > > > > I don't think we need other attributes (well, at least right now). The > > device/vendor name is useful because that's what we expose to the > > userspace for device identification along with the device/vendor ID. > > Okay. It might be worth to log additional attributes with info level. I don't think we want to log anything with info level to be honest. The driver currently already is pretty noisy so adding even more information there just makes it worse ;-) I would rather convert debugging information to use tracepoints and get rid of the tb_*_info() things completely. The whole DROM content is already available through nvm_active/nvmem file under each device (well starting with Alpine Ridge) so the userspace can investigate it as much as it likes without spamming the kernel dmesg :) > > > > +static void tb_drom_parse_generic_entry(struct tb_switch *sw, > > > > + struct tb_drom_entry_generic *entry) > > > > +{ > > > > + if (entry->header.index == 1) > > > > + sw->vendor_name = kstrdup((char *)entry->data, GFP_KERNEL); > > > > + else if (entry->header.index == 2) > > > > + sw->device_name = kstrdup((char *)entry->data, GFP_KERNEL); > > > > +} > > > > > > This assumes that these are properly null-terminated strings, but the DROM > > > may contain complete garbage. The existing drom parser is very careful > > > to validate and sanitize everything. > > > > The DROM specification says they must be null-terminated but I yes, it > > is possible that some of the devices have it wrong. The generic entry > > includes length field so I suppose we can use that + kmemdup() instead > > here? > > Yes, as long as you check that the last character is null. OK, I'll do that then. Thanks.