| Message ID | 20240625185333.23211-4-admiyo@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MCTP over PCC | expand |
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc5 next-20240625]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/admiyo-os-amperecomputing-com/mctp-pcc-Check-before-sending-MCTP-PCC-response-ACK/20240626-052432
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240625185333.23211-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v3 3/3] mctp pcc: Implement MCTP over PCC Transport
config: parisc-allmodconfig (https://download.01.org/0day-ci/archive/20240627/202406270051.3C6XAHU8-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240627/202406270051.3C6XAHU8-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406270051.3C6XAHU8-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:214:9: note: in expansion of macro 'dev_dbg'
214 | dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:215:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
215 | acpi_device_hid(acpi_dev));
| ^~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:224:29: note: in definition of macro '__dynamic_func_call_cls'
224 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:250:9: note: in expansion of macro '_dynamic_func_call_cls'
250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:273:9: note: in expansion of macro '_dynamic_func_call'
273 | _dynamic_func_call(fmt, __dynamic_dev_dbg, \
| ^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:214:9: note: in expansion of macro 'dev_dbg'
214 | dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n",
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:216:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
216 | dev_handle = acpi_device_handle(acpi_dev);
| ^~~~~~~~~~~~~~~~~~
| acpi_device_dep
drivers/net/mctp/mctp-pcc.c:216:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
216 | dev_handle = acpi_device_handle(acpi_dev);
| ^
In file included from include/linux/device.h:15,
from include/linux/acpi.h:14:
drivers/net/mctp/mctp-pcc.c:220:34: error: invalid use of undefined type 'struct acpi_device'
220 | dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
| ^~
include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
drivers/net/mctp/mctp-pcc.c:220:17: note: in expansion of macro 'dev_err'
220 | dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
| ^~~~~~~
drivers/net/mctp/mctp-pcc.c:225:24: error: invalid use of undefined type 'struct acpi_device'
225 | dev = &acpi_dev->dev;
| ^~
drivers/net/mctp/mctp-pcc.c:271:17: error: invalid use of undefined type 'struct acpi_device'
271 | acpi_dev->driver_data = mctp_pcc_dev;
| ^~
drivers/net/mctp/mctp-pcc.c: At top level:
drivers/net/mctp/mctp-pcc.c:326:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
326 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:327:10: error: 'struct acpi_driver' has no member named 'name'
327 | .name = "mctp_pcc",
| ^~~~
drivers/net/mctp/mctp-pcc.c:327:17: warning: excess elements in struct initializer
327 | .name = "mctp_pcc",
| ^~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:327:17: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:328:10: error: 'struct acpi_driver' has no member named 'class'
328 | .class = "Unknown",
| ^~~~~
drivers/net/mctp/mctp-pcc.c:328:18: warning: excess elements in struct initializer
328 | .class = "Unknown",
| ^~~~~~~~~
drivers/net/mctp/mctp-pcc.c:328:18: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:329:10: error: 'struct acpi_driver' has no member named 'ids'
329 | .ids = mctp_pcc_device_ids,
| ^~~
drivers/net/mctp/mctp-pcc.c:329:16: warning: excess elements in struct initializer
329 | .ids = mctp_pcc_device_ids,
| ^~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:329:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:330:10: error: 'struct acpi_driver' has no member named 'ops'
330 | .ops = {
| ^~~
drivers/net/mctp/mctp-pcc.c:330:16: error: extra brace group at end of initializer
330 | .ops = {
| ^
drivers/net/mctp/mctp-pcc.c:330:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:330:16: warning: excess elements in struct initializer
drivers/net/mctp/mctp-pcc.c:330:16: note: (near initialization for 'mctp_pcc_driver')
drivers/net/mctp/mctp-pcc.c:334:10: error: 'struct acpi_driver' has no member named 'owner'
334 | .owner = THIS_MODULE,
| ^~~~~
In file included from arch/parisc/include/asm/alternative.h:18,
from arch/parisc/include/asm/barrier.h:5,
from include/linux/list.h:11,
from include/linux/resource_ext.h:9:
include/linux/init.h:180:21: warning: excess elements in struct initializer
180 | #define THIS_MODULE (&__this_module)
| ^
drivers/net/mctp/mctp-pcc.c:334:18: note: in expansion of macro 'THIS_MODULE'
334 | .owner = THIS_MODULE,
| ^~~~~~~~~~~
include/linux/init.h:180:21: note: (near initialization for 'mctp_pcc_driver')
180 | #define THIS_MODULE (&__this_module)
| ^
drivers/net/mctp/mctp-pcc.c:334:18: note: in expansion of macro 'THIS_MODULE'
334 | .owner = THIS_MODULE,
| ^~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:337:1: warning: data definition has no type or storage class
337 | module_acpi_driver(mctp_pcc_driver);
| ^~~~~~~~~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:337:1: error: type defaults to 'int' in declaration of 'module_acpi_driver' [-Werror=implicit-int]
>> drivers/net/mctp/mctp-pcc.c:337:1: warning: parameter names (without types) in function declaration
drivers/net/mctp/mctp-pcc.c:326:27: error: storage size of 'mctp_pcc_driver' isn't known
326 | static struct acpi_driver mctp_pcc_driver = {
| ^~~~~~~~~~~~~~~
>> drivers/net/mctp/mctp-pcc.c:326:27: warning: 'mctp_pcc_driver' defined but not used [-Wunused-variable]
cc1: some warnings being treated as errors
vim +337 drivers/net/mctp/mctp-pcc.c
325
> 326 static struct acpi_driver mctp_pcc_driver = {
327 .name = "mctp_pcc",
328 .class = "Unknown",
329 .ids = mctp_pcc_device_ids,
330 .ops = {
331 .add = mctp_pcc_driver_add,
332 .remove = mctp_pcc_driver_remove,
333 },
334 .owner = THIS_MODULE,
335 };
336
> 337 module_acpi_driver(mctp_pcc_driver);
338
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.10-rc5 next-20240625]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/admiyo-os-amperecomputing-com/mctp-pcc-Check-before-sending-MCTP-PCC-response-ACK/20240626-052432
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20240625185333.23211-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v3 3/3] mctp pcc: Implement MCTP over PCC Transport
config: powerpc-allyesconfig (https://download.01.org/0day-ci/archive/20240627/202406270625.2MuBj55z-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project ad79a14c9e5ec4a369eed4adf567c22cc029863f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240627/202406270625.2MuBj55z-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406270625.2MuBj55z-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/net/mctp/mctp-pcc.c:8:
In file included from include/linux/if_arp.h:22:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:8:
In file included from include/linux/cacheflush.h:5:
In file included from arch/powerpc/include/asm/cacheflush.h:7:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
500 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
501 | item];
| ~~~~
include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
507 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
508 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
519 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
520 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
528 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
529 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/net/mctp/mctp-pcc.c:17:
include/acpi/acpi_drivers.h:72:43: warning: declaration of 'struct acpi_pci_root' will not be visible outside of this function [-Wvisibility]
72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
| ^
drivers/net/mctp/mctp-pcc.c:214:19: error: incomplete definition of type 'struct acpi_device'
214 | dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n",
| ~~~~~~~~^
include/linux/dev_printk.h:165:18: note: expanded from macro 'dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:274:7: note: expanded from macro 'dynamic_dev_dbg'
274 | dev, fmt, ##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:250:59: note: expanded from macro '_dynamic_func_call'
250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:248:65: note: expanded from macro '_dynamic_func_call_cls'
248 | __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:224:15: note: expanded from macro '__dynamic_func_call_cls'
224 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/acpi.h:792:8: note: forward declaration of 'struct acpi_device'
792 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:215:3: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
215 | acpi_device_hid(acpi_dev));
| ^
drivers/net/mctp/mctp-pcc.c:215:3: note: did you mean 'acpi_device_dep'?
include/acpi/acpi_bus.h:41:6: note: 'acpi_device_dep' declared here
41 | bool acpi_device_dep(acpi_handle target, acpi_handle match);
| ^
drivers/net/mctp/mctp-pcc.c:216:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
216 | dev_handle = acpi_device_handle(acpi_dev);
| ^
drivers/net/mctp/mctp-pcc.c:216:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion]
216 | dev_handle = acpi_device_handle(acpi_dev);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/mctp/mctp-pcc.c:220:20: error: incomplete definition of type 'struct acpi_device'
220 | dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
| ~~~~~~~~^
include/linux/dev_printk.h:154:44: note: expanded from macro 'dev_err'
154 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:110:11: note: expanded from macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/acpi.h:792:8: note: forward declaration of 'struct acpi_device'
792 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:225:17: error: incomplete definition of type 'struct acpi_device'
225 | dev = &acpi_dev->dev;
| ~~~~~~~~^
include/linux/acpi.h:792:8: note: forward declaration of 'struct acpi_device'
792 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:271:10: error: incomplete definition of type 'struct acpi_device'
271 | acpi_dev->driver_data = mctp_pcc_dev;
| ~~~~~~~~^
include/linux/acpi.h:792:8: note: forward declaration of 'struct acpi_device'
792 | struct acpi_device;
| ^
drivers/net/mctp/mctp-pcc.c:326:27: error: variable has incomplete type 'struct acpi_driver'
326 | static struct acpi_driver mctp_pcc_driver = {
| ^
drivers/net/mctp/mctp-pcc.c:326:15: note: forward declaration of 'struct acpi_driver'
326 | static struct acpi_driver mctp_pcc_driver = {
| ^
>> drivers/net/mctp/mctp-pcc.c:337:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
337 | module_acpi_driver(mctp_pcc_driver);
| ^
| int
>> drivers/net/mctp/mctp-pcc.c:337:20: error: a parameter list without types is only allowed in a function definition
337 | module_acpi_driver(mctp_pcc_driver);
| ^
6 warnings and 10 errors generated.
vim +/int +337 drivers/net/mctp/mctp-pcc.c
325
> 326 static struct acpi_driver mctp_pcc_driver = {
327 .name = "mctp_pcc",
328 .class = "Unknown",
329 .ids = mctp_pcc_device_ids,
330 .ops = {
331 .add = mctp_pcc_driver_add,
332 .remove = mctp_pcc_driver_remove,
333 },
334 .owner = THIS_MODULE,
335 };
336
> 337 module_acpi_driver(mctp_pcc_driver);
338
Hi Adam, Looking good, just a couple of minor things inline, and a query on the new _remove() implementation. > +#define SPDM_VERSION_OFFSET 1 > +#define SPDM_REQ_RESP_OFFSET 2 These seem unrelated, and are unused? > +#define MCTP_PAYLOAD_LENGTH 256 > +#define MCTP_CMD_LENGTH 4 > +#define MCTP_PCC_VERSION 0x1 /* DSP0253 defines a single version: 1 */ > +#define MCTP_SIGNATURE "MCTP" > +#define SIGNATURE_LENGTH 4 > +#define MCTP_HEADER_LENGTH 12 > +#define MCTP_MIN_MTU 68 > +#define PCC_MAGIC 0x50434300 > +#define PCC_HEADER_FLAG_REQ_INT 0x1 > +#define PCC_HEADER_FLAGS PCC_HEADER_FLAG_REQ_INT > +#define PCC_DWORD_TYPE 0x0c > +#define PCC_ACK_FLAG_MASK 0x1 > + > +struct mctp_pcc_hdr { > + u32 signature; > + u32 flags; > + u32 length; > + char mctp_signature[4]; > +}; I'd recommend the endian-annotated types for signature, flags & length here, and converting on access. (yes, maybe this will only ever be intended for little-endian, but there's almost always cases where code gets moved around, copied into new drivers, etc) > +struct mctp_pcc_hw_addr { > + u32 inbox_index; > + u32 outbox_index; > +}; > + > +/* The netdev structure. One of these per PCC adapter. */ > +struct mctp_pcc_ndev { > + struct list_head next; > + /* spinlock to serialize access to pcc buffer and registers*/ > + spinlock_t lock; > + struct mctp_dev mdev; > + struct acpi_device *acpi_device; > + struct pcc_mbox_chan *in_chan; > + struct pcc_mbox_chan *out_chan; > + struct mbox_client outbox_client; > + struct mbox_client inbox_client; > + void __iomem *pcc_comm_inbox_addr; > + void __iomem *pcc_comm_outbox_addr; > + struct mctp_pcc_hw_addr hw_addr; > +}; > + > +static struct list_head mctp_pcc_ndevs = LIST_HEAD_INIT(mctp_pcc_ndevs); Saw your changelog entry about needing this, makes sense if that's what the acpi core requires. In which case, you can make this a little more brief with: static LIST_HEAD(mctp_pcc_ndevs); (but more on that below) > +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer) > +{ > + struct mctp_pcc_ndev *mctp_pcc_dev; > + struct mctp_pcc_hdr mctp_pcc_hdr; > + struct mctp_skb_cb *cb; > + struct sk_buff *skb; > + void *skb_buf; > + u32 data_len; > + u32 flags; > + > + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); > + memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr, > + sizeof(struct mctp_pcc_hdr)); > + data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH; > + > + if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) { > + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; > + return; > + } > + > + skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len); > + if (!skb) { > + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; > + return; > + } > + mctp_pcc_dev->mdev.dev->stats.rx_packets++; > + mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len; > + skb->protocol = htons(ETH_P_MCTP); > + skb_buf = skb_put(skb, data_len); > + memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len); > + skb_reset_mac_header(skb); > + skb_pull(skb, sizeof(struct mctp_pcc_hdr)); > + skb_reset_network_header(skb); > + cb = __mctp_cb(skb); > + cb->halen = 0; > + netif_rx(skb); > + > + flags = mctp_pcc_hdr.flags; > + mctp_pcc_dev->in_chan->ack_rx = (flags & PCC_ACK_FLAG_MASK) > 0; The '>' comparison to a bit field is a bit odd, you could just: mctp_pcc_dev->in_chan->ack_rx = !!(flags & PCC_ACK_FLAG_MASK); > +static void > +mctp_pcc_net_stats(struct net_device *net_dev, > + struct rtnl_link_stats64 *stats) > +{ > + struct mctp_pcc_ndev *mpnd; > + > + mpnd = (struct mctp_pcc_ndev *)netdev_priv(net_dev); No need to cast from void *. > +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev) > +{ > + struct lookup_context context = {0, 0, 0}; > + struct mctp_pcc_ndev *mctp_pcc_dev; > + struct net_device *ndev; > + acpi_handle dev_handle; > + acpi_status status; > + struct device *dev; > + int mctp_pcc_mtu; > + int outbox_index; > + int inbox_index; > + char name[32]; > + int rc; > + > + dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n", > + acpi_device_hid(acpi_dev)); > + dev_handle = acpi_device_handle(acpi_dev); > + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, > + &context); > + if (!ACPI_SUCCESS(status)) { > + dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS"); > + return -EINVAL; > + } > + inbox_index = context.inbox_index; > + outbox_index = context.outbox_index; > + dev = &acpi_dev->dev; > + > + snprintf(name, sizeof(name), "mctpipcc%d", inbox_index); > + ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM, > + mctp_pcc_setup); > + if (!ndev) > + return -ENOMEM; > + mctp_pcc_dev = (struct mctp_pcc_ndev *)netdev_priv(ndev); No need for the cast from void *. > +static void mctp_pcc_driver_remove(struct acpi_device *adev) > +{ > + struct list_head *ptr; > + struct list_head *tmp; > + > + list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) { You can save the list_entry() below by using list_for_each_entry_safe() here. > + struct net_device *ndev; > + struct mctp_pcc_ndev *mctp_pcc_dev; > + > + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next); > + if (mctp_pcc_dev->acpi_device != adev) > + continue; Wait, you removed the case where you clean up the whole list if adev is NULL? Now this contradicts your section of the changelog: - Did not change the module init unload function to use the ACPI equivalent as they do not remove all devices prior to unload, leading to dangling references and seg faults. Does this mean you no longer need to free all instances on unload? In which case, that sounds great! that probably also means: - all the list is doing is allowing you to find the mctp_pcc_dev from the acpi_device - but: you can turn an acpi_device into a mctp_pcc_dev from adev->driver_data (which you're already setting), so you don't need the list - then, _remove just becomes something like: static void mctp_pcc_driver_remove(struct acpi_device *adev) { struct mctp_pcc_dev *dev = acpi_driver_data(adev); pcc_mbox_free_channel(dev->out_chan); pcc_mbox_free_channel(dev->in_chan); if (dev->mdev.dev) mctp_unregister_netdev(dev->mdev.dev); } (but I can't see how dev->mdev.dev can be null, so you may be able to remove the condition too) > + pcc_mbox_free_channel(mctp_pcc_dev->out_chan); > + pcc_mbox_free_channel(mctp_pcc_dev->in_chan); > + ndev = mctp_pcc_dev->mdev.dev; > + if (ndev) > + mctp_unregister_netdev(ndev); > + list_del(ptr); > + break; Unusual indent here. Cheers, Jeremy
Yep, you are right, and as I continue to mull this over, I realize I can make it much simpler. Sorry about missing the changelog entry, as I had written that prior to realizing that all of this cleanup was a workaround for the error you found in the first iteration: flipping the null meant I was leaving behind devices when I should have been cleaning them up. So, yeah, I think the list can go now. On 6/28/24 04:41, Jeremy Kerr wrote: > You can save the list_entry() below by using list_for_each_entry_safe() > here. > >> + struct net_device *ndev; >> + struct mctp_pcc_ndev *mctp_pcc_dev; >> + >> + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next); >> + if (mctp_pcc_dev->acpi_device != adev) >> + continue; > Wait, you removed the case where you clean up the whole list if adev is > NULL? > > Now this contradicts your section of the changelog: > > - Did not change the module init unload function to use the > ACPI equivalent as they do not remove all devices prior > to unload, leading to dangling references and seg faults. > > Does this mean you no longer need to free all instances on unload? In > which case, that sounds great! that probably also means: > > - all the list is doing is allowing you to find the mctp_pcc_dev from > the acpi_device > > - but: you can turn an acpi_device into a mctp_pcc_dev from > adev->driver_data (which you're already setting), so you don't need > the list > > - then, _remove just becomes something like: > > static void mctp_pcc_driver_remove(struct acpi_device *adev) > { > struct mctp_pcc_dev *dev = acpi_driver_data(adev); > > pcc_mbox_free_channel(dev->out_chan); > pcc_mbox_free_channel(dev->in_chan); > if (dev->mdev.dev) > mctp_unregister_netdev(dev->mdev.dev); > } > > (but I can't see how dev->mdev.dev can be null, so you may be able > to remove the condition too)
diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig index ce9d2d2ccf3b..ff4effd8e99c 100644 --- a/drivers/net/mctp/Kconfig +++ b/drivers/net/mctp/Kconfig @@ -42,6 +42,19 @@ config MCTP_TRANSPORT_I3C A MCTP protocol network device is created for each I3C bus having a "mctp-controller" devicetree property. +config MCTP_TRANSPORT_PCC + tristate "MCTP PCC transport" + select ACPI + help + Provides a driver to access MCTP devices over PCC transport, + A MCTP protocol network device is created via ACPI for each + entry in the DST/SDST that matches the identifier. The Platform + commuinucation channels are selected from the corresponding + entries in the PCCT. + + Say y here if you need to connect to MCTP endpoints over PCC. To + compile as a module, use m; the module will be called mctp-pcc. + endmenu endif diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile index e1cb99ced54a..492a9e47638f 100644 --- a/drivers/net/mctp/Makefile +++ b/drivers/net/mctp/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c new file mode 100644 index 000000000000..9ef4eefabd58 --- /dev/null +++ b/drivers/net/mctp/mctp-pcc.c @@ -0,0 +1,343 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * mctp-pcc.c - Driver for MCTP over PCC. + * Copyright (c) 2024, Ampere Computing LLC + */ + +#include <linux/acpi.h> +#include <linux/if_arp.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/netdevice.h> +#include <linux/platform_device.h> +#include <linux/string.h> + +#include <acpi/acpi_bus.h> +#include <acpi/acpi_drivers.h> +#include <acpi/acrestyp.h> +#include <acpi/actbl.h> +#include <net/mctp.h> +#include <net/mctpdevice.h> +#include <acpi/pcc.h> + +#define SPDM_VERSION_OFFSET 1 +#define SPDM_REQ_RESP_OFFSET 2 +#define MCTP_PAYLOAD_LENGTH 256 +#define MCTP_CMD_LENGTH 4 +#define MCTP_PCC_VERSION 0x1 /* DSP0253 defines a single version: 1 */ +#define MCTP_SIGNATURE "MCTP" +#define SIGNATURE_LENGTH 4 +#define MCTP_HEADER_LENGTH 12 +#define MCTP_MIN_MTU 68 +#define PCC_MAGIC 0x50434300 +#define PCC_HEADER_FLAG_REQ_INT 0x1 +#define PCC_HEADER_FLAGS PCC_HEADER_FLAG_REQ_INT +#define PCC_DWORD_TYPE 0x0c +#define PCC_ACK_FLAG_MASK 0x1 + +struct mctp_pcc_hdr { + u32 signature; + u32 flags; + u32 length; + char mctp_signature[4]; +}; + +struct mctp_pcc_hw_addr { + u32 inbox_index; + u32 outbox_index; +}; + +/* The netdev structure. One of these per PCC adapter. */ +struct mctp_pcc_ndev { + struct list_head next; + /* spinlock to serialize access to pcc buffer and registers*/ + spinlock_t lock; + struct mctp_dev mdev; + struct acpi_device *acpi_device; + struct pcc_mbox_chan *in_chan; + struct pcc_mbox_chan *out_chan; + struct mbox_client outbox_client; + struct mbox_client inbox_client; + void __iomem *pcc_comm_inbox_addr; + void __iomem *pcc_comm_outbox_addr; + struct mctp_pcc_hw_addr hw_addr; +}; + +static struct list_head mctp_pcc_ndevs = LIST_HEAD_INIT(mctp_pcc_ndevs); + +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer) +{ + struct mctp_pcc_ndev *mctp_pcc_dev; + struct mctp_pcc_hdr mctp_pcc_hdr; + struct mctp_skb_cb *cb; + struct sk_buff *skb; + void *skb_buf; + u32 data_len; + u32 flags; + + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client); + memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr, + sizeof(struct mctp_pcc_hdr)); + data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH; + + if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) { + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; + return; + } + + skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len); + if (!skb) { + mctp_pcc_dev->mdev.dev->stats.rx_dropped++; + return; + } + mctp_pcc_dev->mdev.dev->stats.rx_packets++; + mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len; + skb->protocol = htons(ETH_P_MCTP); + skb_buf = skb_put(skb, data_len); + memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len); + skb_reset_mac_header(skb); + skb_pull(skb, sizeof(struct mctp_pcc_hdr)); + skb_reset_network_header(skb); + cb = __mctp_cb(skb); + cb->halen = 0; + netif_rx(skb); + + flags = mctp_pcc_hdr.flags; + mctp_pcc_dev->in_chan->ack_rx = (flags & PCC_ACK_FLAG_MASK) > 0; +} + +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev) +{ + struct mctp_pcc_hdr pcc_header; + struct mctp_pcc_ndev *mpnd; + void __iomem *buffer; + unsigned long flags; + + ndev->stats.tx_bytes += skb->len; + ndev->stats.tx_packets++; + mpnd = netdev_priv(ndev); + + spin_lock_irqsave(&mpnd->lock, flags); + buffer = mpnd->pcc_comm_outbox_addr; + pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index; + pcc_header.flags = PCC_HEADER_FLAGS; + memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH); + pcc_header.length = skb->len + SIGNATURE_LENGTH; + memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr)); + memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len); + mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, + NULL); + spin_unlock_irqrestore(&mpnd->lock, flags); + + dev_consume_skb_any(skb); + return NETDEV_TX_OK; +} + +static void +mctp_pcc_net_stats(struct net_device *net_dev, + struct rtnl_link_stats64 *stats) +{ + struct mctp_pcc_ndev *mpnd; + + mpnd = (struct mctp_pcc_ndev *)netdev_priv(net_dev); + stats->rx_errors = 0; + stats->rx_packets = mpnd->mdev.dev->stats.rx_packets; + stats->tx_packets = mpnd->mdev.dev->stats.tx_packets; + stats->rx_dropped = 0; + stats->tx_bytes = mpnd->mdev.dev->stats.tx_bytes; + stats->rx_bytes = mpnd->mdev.dev->stats.rx_bytes; +} + +static const struct net_device_ops mctp_pcc_netdev_ops = { + .ndo_start_xmit = mctp_pcc_tx, + .ndo_get_stats64 = mctp_pcc_net_stats, +}; + +static void mctp_pcc_setup(struct net_device *ndev) +{ + ndev->type = ARPHRD_MCTP; + ndev->hard_header_len = 0; + ndev->addr_len = 0; + ndev->tx_queue_len = 0; + ndev->flags = IFF_NOARP; + ndev->netdev_ops = &mctp_pcc_netdev_ops; + ndev->needs_free_netdev = true; +} + +struct lookup_context { + int index; + u32 inbox_index; + u32 outbox_index; +}; + +static acpi_status lookup_pcct_indices(struct acpi_resource *ares, + void *context) +{ + struct acpi_resource_address32 *addr; + struct lookup_context *luc = context; + + switch (ares->type) { + case PCC_DWORD_TYPE: + break; + default: + return AE_OK; + } + + addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data); + switch (luc->index) { + case 0: + luc->outbox_index = addr[0].address.minimum; + break; + case 1: + luc->inbox_index = addr[0].address.minimum; + break; + } + luc->index++; + return AE_OK; +} + +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev) +{ + struct lookup_context context = {0, 0, 0}; + struct mctp_pcc_ndev *mctp_pcc_dev; + struct net_device *ndev; + acpi_handle dev_handle; + acpi_status status; + struct device *dev; + int mctp_pcc_mtu; + int outbox_index; + int inbox_index; + char name[32]; + int rc; + + dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID %s\n", + acpi_device_hid(acpi_dev)); + dev_handle = acpi_device_handle(acpi_dev); + status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, + &context); + if (!ACPI_SUCCESS(status)) { + dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS"); + return -EINVAL; + } + inbox_index = context.inbox_index; + outbox_index = context.outbox_index; + dev = &acpi_dev->dev; + + snprintf(name, sizeof(name), "mctpipcc%d", inbox_index); + ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM, + mctp_pcc_setup); + if (!ndev) + return -ENOMEM; + mctp_pcc_dev = (struct mctp_pcc_ndev *)netdev_priv(ndev); + INIT_LIST_HEAD(&mctp_pcc_dev->next); + spin_lock_init(&mctp_pcc_dev->lock); + + mctp_pcc_dev->hw_addr.inbox_index = inbox_index; + mctp_pcc_dev->hw_addr.outbox_index = outbox_index; + mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback; + mctp_pcc_dev->out_chan = + pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client, + outbox_index); + if (IS_ERR(mctp_pcc_dev->out_chan)) { + rc = PTR_ERR(mctp_pcc_dev->out_chan); + goto free_netdev; + } + mctp_pcc_dev->in_chan = + pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client, + inbox_index); + if (IS_ERR(mctp_pcc_dev->in_chan)) { + rc = PTR_ERR(mctp_pcc_dev->in_chan); + goto cleanup_out_channel; + } + mctp_pcc_dev->pcc_comm_inbox_addr = + devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr, + mctp_pcc_dev->in_chan->shmem_size); + if (!mctp_pcc_dev->pcc_comm_inbox_addr) { + rc = -EINVAL; + goto cleanup_in_channel; + } + mctp_pcc_dev->pcc_comm_outbox_addr = + devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr, + mctp_pcc_dev->out_chan->shmem_size); + if (!mctp_pcc_dev->pcc_comm_outbox_addr) { + rc = -EINVAL; + goto cleanup_in_channel; + } + mctp_pcc_dev->acpi_device = acpi_dev; + mctp_pcc_dev->inbox_client.dev = dev; + mctp_pcc_dev->outbox_client.dev = dev; + mctp_pcc_dev->mdev.dev = ndev; + acpi_dev->driver_data = mctp_pcc_dev; + + /* There is no clean way to pass the MTU + * to the callback function used for registration, + * so set the values ahead of time. + */ + mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size - + sizeof(struct mctp_pcc_hdr); + ndev->mtu = MCTP_MIN_MTU; + ndev->max_mtu = mctp_pcc_mtu; + ndev->min_mtu = MCTP_MIN_MTU; + + rc = register_netdev(ndev); + if (rc) + goto cleanup_in_channel; + list_add_tail(&mctp_pcc_dev->next, &mctp_pcc_ndevs); + return 0; + +cleanup_in_channel: + pcc_mbox_free_channel(mctp_pcc_dev->in_chan); +cleanup_out_channel: + pcc_mbox_free_channel(mctp_pcc_dev->out_chan); +free_netdev: + unregister_netdev(ndev); + free_netdev(ndev); + return rc; +} + +static void mctp_pcc_driver_remove(struct acpi_device *adev) +{ + struct list_head *ptr; + struct list_head *tmp; + + list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) { + struct net_device *ndev; + struct mctp_pcc_ndev *mctp_pcc_dev; + + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next); + if (mctp_pcc_dev->acpi_device != adev) + continue; + pcc_mbox_free_channel(mctp_pcc_dev->out_chan); + pcc_mbox_free_channel(mctp_pcc_dev->in_chan); + ndev = mctp_pcc_dev->mdev.dev; + if (ndev) + mctp_unregister_netdev(ndev); + list_del(ptr); + break; + } +} + +static const struct acpi_device_id mctp_pcc_device_ids[] = { + { "DMT0001", 0}, + { "", 0}, +}; + +static struct acpi_driver mctp_pcc_driver = { + .name = "mctp_pcc", + .class = "Unknown", + .ids = mctp_pcc_device_ids, + .ops = { + .add = mctp_pcc_driver_add, + .remove = mctp_pcc_driver_remove, + }, + .owner = THIS_MODULE, +}; + +module_acpi_driver(mctp_pcc_driver); + +MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids); + +MODULE_DESCRIPTION("MCTP PCC device"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");