From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Vesker Subject: Re: [PATCH net-next 0/9] devlink: Add support for region access Date: Fri, 30 Mar 2018 08:28:39 +0300 Message-ID: <28b99a08-1967-3044-4010-0faa5d6bfc14@mellanox.com> References: <1522339672-18273-1-git-send-email-valex@mellanox.com> <20180329171359.GA12150@lunn.ch> <962b56c1-d471-97ec-e8e9-18252e809dfe@mellanox.com> <20180329195154.GB15565@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: "David S. Miller" , , "Tariq Toukan" , Jiri Pirko To: Andrew Lunn Return-path: Received: from mail-db5eur01on0070.outbound.protection.outlook.com ([104.47.2.70]:40104 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751017AbeC3F2s (ORCPT ); Fri, 30 Mar 2018 01:28:48 -0400 In-Reply-To: <20180329195154.GB15565@lunn.ch> Sender: netdev-owner@vger.kernel.org List-ID: On 3/29/2018 10:51 PM, Andrew Lunn wrote: >>>> Show all of the exposed regions with region sizes: >>>> $ devlink region show >>>> pci/0000:00:05.0/cr-space: size 1048576 snapshot [1 2] >>> So you have 2Mbytes of snapshot data. Is this held in the device, or >>> kernel memory? >> This is allocated in devlink, the maximum number of snapshots is set by the >> driver. > And it seems to want contiguous pages. How well does that work after > the system has been running for a while and memory is fragmented? The allocation can be changed, there is no read need for contiguous pages. It is important to note that we the amount of snapshots is limited by the driver this can be based on the dump size or expected frequency of collection. I also prefer not to pre-allocate this memory. >>>> Dump a snapshot: >>>> $ devlink region dump pci/0000:00:05.0/fw-health snapshot 1 >>>> 0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30 >>>> 0000000000000010 0000 0000 ffff ff04 0029 8c00 0028 8cc8 >>>> 0000000000000020 0016 0bb8 0016 1720 0000 0000 c00f 3ffc >>>> 0000000000000030 bada cce5 bada cce5 bada cce5 bada cce5 >>>> >>>> Read a specific part of a snapshot: >>>> $ devlink region read pci/0000:00:05.0/fw-health snapshot 1 address 0 >>>> length 16 >>>> 0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30 >>> Why a separate command? It seems to be just a subset of dump. >> This is useful when debugging values on specific addresses, this also >> brings the API one step closer for a read and write API. > The functionality is useful, yes. But why two commands? Why not one > command, dump, which takes optional parameters? Dump in devlink means provide all the data, saying dump address x length y sounds confusing. Do you see this as a critical issue? > Also, i doubt write support will be accepted. That sounds like the > start of an API to allow a user space driver. If this will be an issue we will stay with read access only. > > Andrew