| Message ID | 20200204082408.18728-2-liweihang@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ib core support to send ib port link event | expand |
On Tue, Feb 04, 2020 at 04:24:02PM +0800, Weihang Li wrote: > From: Lang Cheng <chenglang@huawei.com> > > Add attribute inactive to mark bonding backup port. > > Signed-off-by: Lang Cheng <chenglang@huawei.com> > drivers/infiniband/core/cache.c | 16 +++++++++++++++- > include/rdma/ib_cache.h | 10 ++++++++++ > include/rdma/ib_verbs.h | 2 ++ > 3 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > index d535995..7a7ef0e 100644 > +++ b/drivers/infiniband/core/cache.c > @@ -1175,6 +1175,19 @@ int ib_get_cached_port_state(struct ib_device *device, > } > EXPORT_SYMBOL(ib_get_cached_port_state); > > +u8 ib_get_cached_port_inactive_status(struct ib_device *device, u8 port_num) > +{ > + unsigned long flags; > + u8 inactive; > + > + read_lock_irqsave(&device->cache.lock, flags); > + inactive = device->port_data[port_num].cache.inactive; > + read_unlock_irqrestore(&device->cache.lock, flags); > + > + return inactive; > +} > +EXPORT_SYMBOL(ib_get_cached_port_inactive_status); > + > /** > * rdma_get_gid_attr - Returns GID attributes for a port of a device > * at a requested gid_index, if a valid GID entry exists. > @@ -1393,7 +1406,7 @@ static void ib_cache_update(struct ib_device *device, > if (!rdma_is_port_valid(device, port)) > return; > > - tprops = kmalloc(sizeof *tprops, GFP_KERNEL); > + tprops = kzalloc(sizeof *tprops, GFP_KERNEL); > if (!tprops) > return; > > @@ -1435,6 +1448,7 @@ static void ib_cache_update(struct ib_device *device, > device->port_data[port].cache.pkey = pkey_cache; > device->port_data[port].cache.lmc = tprops->lmc; > device->port_data[port].cache.port_state = tprops->state; > + device->port_data[port].cache.inactive = tprops->inactive; > > device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix; > write_unlock_irq(&device->cache.lock); > diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h > index 870b5e6..63b2dd6 100644 > +++ b/include/rdma/ib_cache.h > @@ -131,6 +131,16 @@ int ib_get_cached_port_state(struct ib_device *device, > u8 port_num, > enum ib_port_state *port_active); > > +/** > + * ib_get_cached_port_inactive_status - Returns a cached port inactive status > + * @device: The device to query. > + * @port_num: The port number of the device to query. > + * > + * ib_get_cached_port_inactive_status() fetches the specified event inactive > + * status stored in the local software cache. > + */ > +u8 ib_get_cached_port_inactive_status(struct ib_device *device, u8 port_num); > + kdocs belong with the implementation, not in the header file > bool rdma_is_zero_gid(const union ib_gid *gid); > const struct ib_gid_attr *rdma_get_gid_attr(struct ib_device *device, > u8 port_num, int index); > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index 5608e14..e17d846 100644 > +++ b/include/rdma/ib_verbs.h > @@ -665,6 +665,7 @@ struct ib_port_attr { > u8 active_speed; > u8 phys_state; > u16 port_cap_flags2; > + u8 inactive; > }; Why is a major structure being changed for this? If LAG is to be brought up to the core code then the core should know what leg is active or not, not the driver. > enum ib_device_modify_flags { > @@ -2145,6 +2146,7 @@ struct ib_port_cache { > struct ib_gid_table *gid; > u8 lmc; > enum ib_port_state port_state; > + u8 inactive; > }; Please think carefully about structure patcking here, both placements of u8 seem poor Jason
On 2020/2/20 5:01, Jason Gunthorpe wrote: > On Tue, Feb 04, 2020 at 04:24:02PM +0800, Weihang Li wrote: >> From: Lang Cheng <chenglang@huawei.com> >> >> Add attribute inactive to mark bonding backup port. >> >> Signed-off-by: Lang Cheng <chenglang@huawei.com> >> drivers/infiniband/core/cache.c | 16 +++++++++++++++- >> include/rdma/ib_cache.h | 10 ++++++++++ >> include/rdma/ib_verbs.h | 2 ++ >> 3 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c >> index d535995..7a7ef0e 100644 >> +++ b/drivers/infiniband/core/cache.c >> @@ -1175,6 +1175,19 @@ int ib_get_cached_port_state(struct ib_device *device, >> } >> EXPORT_SYMBOL(ib_get_cached_port_state); >> >> +u8 ib_get_cached_port_inactive_status(struct ib_device *device, u8 port_num) >> +{ >> + unsigned long flags; >> + u8 inactive; >> + >> + read_lock_irqsave(&device->cache.lock, flags); >> + inactive = device->port_data[port_num].cache.inactive; >> + read_unlock_irqrestore(&device->cache.lock, flags); >> + >> + return inactive; >> +} >> +EXPORT_SYMBOL(ib_get_cached_port_inactive_status); >> + >> /** >> * rdma_get_gid_attr - Returns GID attributes for a port of a device >> * at a requested gid_index, if a valid GID entry exists. >> @@ -1393,7 +1406,7 @@ static void ib_cache_update(struct ib_device *device, >> if (!rdma_is_port_valid(device, port)) >> return; >> >> - tprops = kmalloc(sizeof *tprops, GFP_KERNEL); >> + tprops = kzalloc(sizeof *tprops, GFP_KERNEL); >> if (!tprops) >> return; >> >> @@ -1435,6 +1448,7 @@ static void ib_cache_update(struct ib_device *device, >> device->port_data[port].cache.pkey = pkey_cache; >> device->port_data[port].cache.lmc = tprops->lmc; >> device->port_data[port].cache.port_state = tprops->state; >> + device->port_data[port].cache.inactive = tprops->inactive; >> >> device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix; >> write_unlock_irq(&device->cache.lock); >> diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h >> index 870b5e6..63b2dd6 100644 >> +++ b/include/rdma/ib_cache.h >> @@ -131,6 +131,16 @@ int ib_get_cached_port_state(struct ib_device *device, >> u8 port_num, >> enum ib_port_state *port_active); >> >> +/** >> + * ib_get_cached_port_inactive_status - Returns a cached port inactive status >> + * @device: The device to query. >> + * @port_num: The port number of the device to query. >> + * >> + * ib_get_cached_port_inactive_status() fetches the specified event inactive >> + * status stored in the local software cache. >> + */ >> +u8 ib_get_cached_port_inactive_status(struct ib_device *device, u8 port_num); >> + > > kdocs belong with the implementation, not in the header file All of ib_get_cached_XXX() should remove rdma_is_port_valid() and move kdocs from ib_cache.h to cache.c. I can send this first, independently. > >> bool rdma_is_zero_gid(const union ib_gid *gid); >> const struct ib_gid_attr *rdma_get_gid_attr(struct ib_device *device, >> u8 port_num, int index); >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >> index 5608e14..e17d846 100644 >> +++ b/include/rdma/ib_verbs.h >> @@ -665,6 +665,7 @@ struct ib_port_attr { >> u8 active_speed; >> u8 phys_state; >> u16 port_cap_flags2; >> + u8 inactive; >> }; > > Why is a major structure being changed for this? > > If LAG is to be brought up to the core code then the core should know > what leg is active or not, not the driver. "LAG is to be brought up to the core", is this under planning? The link event report could be after this . Then, there may be a better way to ignore inactive port. > >> enum ib_device_modify_flags { >> @@ -2145,6 +2146,7 @@ struct ib_port_cache { >> struct ib_gid_table *gid; >> u8 lmc; >> enum ib_port_state port_state; >> + u8 inactive; >> }; > > Please think carefully about structure patcking here, both placements > of u8 seem poor > > Jason > > . >
On Thu, Feb 20, 2020 at 11:19:25AM +0800, Lang Cheng wrote: > > > On 2020/2/20 5:01, Jason Gunthorpe wrote: > > On Tue, Feb 04, 2020 at 04:24:02PM +0800, Weihang Li wrote: > > > From: Lang Cheng <chenglang@huawei.com> > > > > > > Add attribute inactive to mark bonding backup port. > > > > > > Signed-off-by: Lang Cheng <chenglang@huawei.com> > > > drivers/infiniband/core/cache.c | 16 +++++++++++++++- > > > include/rdma/ib_cache.h | 10 ++++++++++ > > > include/rdma/ib_verbs.h | 2 ++ > > > 3 files changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > > > index d535995..7a7ef0e 100644 > > > +++ b/drivers/infiniband/core/cache.c > > > @@ -1175,6 +1175,19 @@ int ib_get_cached_port_state(struct ib_device *device, > > > } > > > EXPORT_SYMBOL(ib_get_cached_port_state); > > > +u8 ib_get_cached_port_inactive_status(struct ib_device *device, u8 port_num) > > > +{ > > > + unsigned long flags; > > > + u8 inactive; > > > + > > > + read_lock_irqsave(&device->cache.lock, flags); > > > + inactive = device->port_data[port_num].cache.inactive; > > > + read_unlock_irqrestore(&device->cache.lock, flags); > > > + > > > + return inactive; > > > +} > > > +EXPORT_SYMBOL(ib_get_cached_port_inactive_status); > > > + > > > /** > > > * rdma_get_gid_attr - Returns GID attributes for a port of a device > > > * at a requested gid_index, if a valid GID entry exists. > > > @@ -1393,7 +1406,7 @@ static void ib_cache_update(struct ib_device *device, > > > if (!rdma_is_port_valid(device, port)) > > > return; > > > - tprops = kmalloc(sizeof *tprops, GFP_KERNEL); > > > + tprops = kzalloc(sizeof *tprops, GFP_KERNEL); > > > if (!tprops) > > > return; > > > @@ -1435,6 +1448,7 @@ static void ib_cache_update(struct ib_device *device, > > > device->port_data[port].cache.pkey = pkey_cache; > > > device->port_data[port].cache.lmc = tprops->lmc; > > > device->port_data[port].cache.port_state = tprops->state; > > > + device->port_data[port].cache.inactive = tprops->inactive; > > > device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix; > > > write_unlock_irq(&device->cache.lock); > > > diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h > > > index 870b5e6..63b2dd6 100644 > > > +++ b/include/rdma/ib_cache.h > > > @@ -131,6 +131,16 @@ int ib_get_cached_port_state(struct ib_device *device, > > > u8 port_num, > > > enum ib_port_state *port_active); > > > +/** > > > + * ib_get_cached_port_inactive_status - Returns a cached port inactive status > > > + * @device: The device to query. > > > + * @port_num: The port number of the device to query. > > > + * > > > + * ib_get_cached_port_inactive_status() fetches the specified event inactive > > > + * status stored in the local software cache. > > > + */ > > > +u8 ib_get_cached_port_inactive_status(struct ib_device *device, u8 port_num); > > > + > > > > kdocs belong with the implementation, not in the header file > > All of ib_get_cached_XXX() should remove rdma_is_port_valid() and move kdocs > from ib_cache.h to cache.c. > I can send this first, independently. > > > > > > bool rdma_is_zero_gid(const union ib_gid *gid); > > > const struct ib_gid_attr *rdma_get_gid_attr(struct ib_device *device, > > > u8 port_num, int index); > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > index 5608e14..e17d846 100644 > > > +++ b/include/rdma/ib_verbs.h > > > @@ -665,6 +665,7 @@ struct ib_port_attr { > > > u8 active_speed; > > > u8 phys_state; > > > u16 port_cap_flags2; > > > + u8 inactive; > > > }; > > > > Why is a major structure being changed for this? > > > > If LAG is to be brought up to the core code then the core should know > > what leg is active or not, not the driver. > > "LAG is to be brought up to the core", is this under planning? It is a development direction, sooner or later that will happen. Thanks
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index d535995..7a7ef0e 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -1175,6 +1175,19 @@ int ib_get_cached_port_state(struct ib_device *device, } EXPORT_SYMBOL(ib_get_cached_port_state); +u8 ib_get_cached_port_inactive_status(struct ib_device *device, u8 port_num) +{ + unsigned long flags; + u8 inactive; + + read_lock_irqsave(&device->cache.lock, flags); + inactive = device->port_data[port_num].cache.inactive; + read_unlock_irqrestore(&device->cache.lock, flags); + + return inactive; +} +EXPORT_SYMBOL(ib_get_cached_port_inactive_status); + /** * rdma_get_gid_attr - Returns GID attributes for a port of a device * at a requested gid_index, if a valid GID entry exists. @@ -1393,7 +1406,7 @@ static void ib_cache_update(struct ib_device *device, if (!rdma_is_port_valid(device, port)) return; - tprops = kmalloc(sizeof *tprops, GFP_KERNEL); + tprops = kzalloc(sizeof *tprops, GFP_KERNEL); if (!tprops) return; @@ -1435,6 +1448,7 @@ static void ib_cache_update(struct ib_device *device, device->port_data[port].cache.pkey = pkey_cache; device->port_data[port].cache.lmc = tprops->lmc; device->port_data[port].cache.port_state = tprops->state; + device->port_data[port].cache.inactive = tprops->inactive; device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix; write_unlock_irq(&device->cache.lock); diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h index 870b5e6..63b2dd6 100644 --- a/include/rdma/ib_cache.h +++ b/include/rdma/ib_cache.h @@ -131,6 +131,16 @@ int ib_get_cached_port_state(struct ib_device *device, u8 port_num, enum ib_port_state *port_active); +/** + * ib_get_cached_port_inactive_status - Returns a cached port inactive status + * @device: The device to query. + * @port_num: The port number of the device to query. + * + * ib_get_cached_port_inactive_status() fetches the specified event inactive + * status stored in the local software cache. + */ +u8 ib_get_cached_port_inactive_status(struct ib_device *device, u8 port_num); + bool rdma_is_zero_gid(const union ib_gid *gid); const struct ib_gid_attr *rdma_get_gid_attr(struct ib_device *device, u8 port_num, int index); diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 5608e14..e17d846 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -665,6 +665,7 @@ struct ib_port_attr { u8 active_speed; u8 phys_state; u16 port_cap_flags2; + u8 inactive; }; enum ib_device_modify_flags { @@ -2145,6 +2146,7 @@ struct ib_port_cache { struct ib_gid_table *gid; u8 lmc; enum ib_port_state port_state; + u8 inactive; }; struct ib_cache {