On 10/06/25 03:29PM, Jonathan Cameron wrote:
On Mon, 2 Jun 2025 19:29:41 +0530
Arpit Kumar <arpit1.ku...@samsung.com> wrote:
Modified Identify Switch Device (Opcode 5100h)
& Get Physical Port State(Opcode 5101h)
using physical ports info stored during enumeration
Signed-off-by: Arpit Kumar <arpit1.ku...@samsung.com>
A few additional comments in here.
J
---
hw/cxl/cxl-mailbox-utils.c | 133 +++++++------------------------------
1 file changed, 24 insertions(+), 109 deletions(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 680055c6c0..b2fa79a721 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -558,17 +558,7 @@ static CXLRetCode cmd_set_response_msg_limit(const struct
cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
-static void cxl_set_dsp_active_bm(PCIBus *b, PCIDevice *d,
- void *private)
-{
- uint8_t *bm = private;
- if (object_dynamic_cast(OBJECT(d), TYPE_CXL_DSP)) {
- uint8_t port = PCIE_PORT(d)->port;
- bm[port / 8] |= 1 << (port % 8);
- }
-}
-
-/* CXL r3.1 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
+/* CXL r3.2 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
I'd prefer the spec reference updates in a separate patch. They are noise here
and kind of suggest there are real changes rather than just refactoring.
okay
@@ -611,16 +599,14 @@ static CXLRetCode cmd_identify_switch_device(const struct
cxl_cmd *cmd,
out->ingress_port_id = 0;
}
- pci_for_each_device_under_bus(bus, cxl_set_dsp_active_bm,
- out->active_port_bitmask);
- out->active_port_bitmask[usp->port / 8] |= (1 << usp->port % 8);
Ah. With this in front of me the reason for the sizeing is much clearer
than in previous patch on it's own. Combining the two will make it all more
obvious.
right, will do the same in next iteration(V2) of the patch series.
-
+ memcpy(out->active_port_bitmask, cci->pports.active_port_bitmask,
+ sizeof(cci->pports.active_port_bitmask));
*len_out = sizeof(*out);
return CXL_MBOX_SUCCESS;
}
-/* CXL r3.1 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
+/* CXL r3.2 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
uint8_t *payload_in,
size_t len_in,
@@ -628,44 +614,21 @@ static CXLRetCode cmd_get_physical_port_state(const
struct cxl_cmd *cmd,
size_t *len_out,
CXLCCI *cci)
{
in = (struct cxl_fmapi_get_phys_port_state_req_pl *)payload_in;
out = (struct cxl_fmapi_get_phys_port_state_resp_pl *)payload_out;
@@ -673,72 +636,24 @@ static CXLRetCode cmd_get_physical_port_state(const
struct cxl_cmd *cmd,
if (len_in < sizeof(*in)) {
return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
}
- /* Check if what was requested can fit */
+
The check is still here... So why remove the comment?
thanks for pointing this out, will add the comment back.
if (sizeof(*out) + sizeof(*out->ports) * in->num_ports > cci->payload_max)
{
return CXL_MBOX_INVALID_INPUT;
}
- /* For success there should be a match for each requested */
- out->num_ports = in->num_ports;
+ if (in->num_ports > cci->pports.num_ports) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+ out->num_ports = in->num_ports;
for (i = 0; i < in->num_ports; i++) {
- struct cxl_fmapi_port_state_info_block *port;
- /* First try to match on downstream port */
- PCIDevice *port_dev;
- uint16_t lnkcap, lnkcap2, lnksta;
-
- port = &out->ports[i];
-
- port_dev = pcie_find_port_by_pn(bus, in->ports[i]);
- if (port_dev) { /* DSP */
- PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
- ->devices[0];
- port->config_state = 3;
- if (ds_dev) {
- if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
- port->connected_device_type = 5; /* Assume MLD for now */
- } else {
- port->connected_device_type = 1;
- }
- } else {
- port->connected_device_type = 0;
+ int pn = in->ports[i];
+ for (int j = 0; j < PCI_DEVFN_MAX; j++) {
+ if (pn == cci->pports.pport_info[j].port_id) {
Given port id is 0-255 and your port_info has 256 elements, why not index
by port_id when storing them in the first place? That should reduce
complexity of this look up. I don't think we ever actually look up
by devfn?
okay
+ memcpy(&out->ports[i], &(cci->pports.pport_info[pn]),
+ sizeof(struct cxl_phy_port_info));
