diff mbox

[04/37] IB/rdmavt: Add ib core device attributes to rvt driver params list

Message ID 20151207204314.8144.46170.stgit@phlsvslse11.ph.intel.com (mailing list archive)
State Superseded
Headers show

Commit Message

Dennis Dalessandro Dec. 7, 2015, 8:43 p.m. UTC 
Instead of trying to handle each parameter separately, add ib_device_attr
to rvt_driver_params. This means drivers will fill this in and pass to the
rvt registration function.

Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/sw/rdmavt/pd.c |    2 +
 include/rdma/rdma_vt.h            |   60 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 3 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Haggai Eran Dec. 10, 2015, 12:26 p.m. UTC | #1 
On 07/12/2015 22:43, Dennis Dalessandro wrote:
> +	/*
> +	 * Drivers will need to support a number of notifications to rvt in
> +	 * accordance with certain events. This structure should contain a mask
> +	 * of the supported events. Such events that the rvt may need to know
> +	 * about include:
> +	 * port errors
> +	 * port active
> +	 * lid change
> +	 * sm change
> +	 * client reregister
> +	 * pkey change
Where are the event values defined?

> +	 *
> +	 * There may also be other events that the rvt layers needs to know
> +	 * about this is not an exhaustive list. Some events though rvt does not
> +	 * need to rely on the driver for such as completion queue error.
> +	 */
> +	 int rvt_signal_supported;

What will rvt do if it learns that the device doesn't support some of
the events?

Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Haggai Eran Dec. 10, 2015, 12:29 p.m. UTC | #2 
On 07/12/2015 22:43, Dennis Dalessandro wrote:
>  struct rvt_dev_info {
> +	/*
> +	 * Prior to calling for registration the driver will be responsible for
> +	 * allocating space for this structure. The driver will also need to
> +	 * allocate space for any private device or per port data structures.
> +	 * Alternatively rvt could do this allocation and the registration API
> +	 * would then change to accept an "extra" piece to allocate.
I don't think you need rvt to allocate the private data, but even if you
decide to do that, there's no need for a comment that describes all the
alternative designs here.

> +	 *
> +	 * The driver will also be
> +	 * responsible for filling in certain members of dparms.props
> +	 */

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dennis Dalessandro Dec. 10, 2015, 1:25 p.m. UTC | #3 
On Thu, Dec 10, 2015 at 02:26:11PM +0200, Haggai Eran wrote:
>On 07/12/2015 22:43, Dennis Dalessandro wrote:
>> +	/*
>> +	 * Drivers will need to support a number of notifications to rvt in
>> +	 * accordance with certain events. This structure should contain a mask
>> +	 * of the supported events. Such events that the rvt may need to know
>> +	 * about include:
>> +	 * port errors
>> +	 * port active
>> +	 * lid change
>> +	 * sm change
>> +	 * client reregister
>> +	 * pkey change
>Where are the event values defined?

They aren't really yet. A lot of the comments like this were put in and made 
available on GitHub for an early look into the design. This will all get 
cleaned up as the code continues to come together.

>>> +	 *
>> +	 * There may also be other events that the rvt layers needs to know
>> +	 * about this is not an exhaustive list. Some events though rvt does not
>> +	 * need to rely on the driver for such as completion queue error.
>> +	 */
>> +	 int rvt_signal_supported;
>
>What will rvt do if it learns that the device doesn't support some of
>the events?

Good question. I don't really have a great answer right now. Mostly it sort 
of depends on what events we end up. If it's something critical rdmavt will 
have to check during the registration and fail the call. This is not yet 
implemented in the two patch sets posted.

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dennis Dalessandro Dec. 10, 2015, 1:35 p.m. UTC | #4 
On Thu, Dec 10, 2015 at 02:29:18PM +0200, Haggai Eran wrote:
>On 07/12/2015 22:43, Dennis Dalessandro wrote:
>>  struct rvt_dev_info {
>> +	/*
>> +	 * Prior to calling for registration the driver will be responsible for
>> +	 * allocating space for this structure. The driver will also need to
>> +	 * allocate space for any private device or per port data structures.
>> +	 * Alternatively rvt could do this allocation and the registration API
>> +	 * would then change to accept an "extra" piece to allocate.
>I don't think you need rvt to allocate the private data, but even if you
>decide to do that, there's no need for a comment that describes all the
>alternative designs here.

Agree. 

This is another of those comments which was meant to provide an early look 
at the design (and alternatives) when I first posted to GitHub.  The 
underlying issue here is who allocates the rvt_dev_info.  

The approach I went with was having the driver do the allocation. This also 
means there is no need for any "private" data in the rvt_dev_info structure.  
If drivers need some private struct they can just create a data structure 
and embed both rvt_dev_info and that private data. See struct qib_ibdev.

This comment needs updated/removed.

>
>> +	 *
>> +	 * The driver will also be
>> +	 * responsible for filling in certain members of dparms.props
>> +	 */
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Haggai Eran Dec. 10, 2015, 3:12 p.m. UTC | #5 
On 10/12/2015 15:25, Dennis Dalessandro wrote:
> On Thu, Dec 10, 2015 at 02:26:11PM +0200, Haggai Eran wrote:
>> On 07/12/2015 22:43, Dennis Dalessandro wrote:
>>> +    /*
>>> +     * Drivers will need to support a number of notifications to rvt in
>>> +     * accordance with certain events. This structure should contain
>>> a mask
>>> +     * of the supported events. Such events that the rvt may need to
>>> know
>>> +     * about include:
>>> +     * port errors
>>> +     * port active
>>> +     * lid change
>>> +     * sm change
>>> +     * client reregister
>>> +     * pkey change
>> Where are the event values defined?
> 
> They aren't really yet. A lot of the comments like this were put in and
> made available on GitHub for an early look into the design. This will
> all get cleaned up as the code continues to come together.
Okay, Great. Since the events above are already defined as
ib_event/ib_event_type maybe you can reuse them somehow.

>>>> +     *
>>> +     * There may also be other events that the rvt layers needs to know
>>> +     * about this is not an exhaustive list. Some events though rvt
>>> does not
>>> +     * need to rely on the driver for such as completion queue error.
>>> +     */
>>> +     int rvt_signal_supported;
>>
>> What will rvt do if it learns that the device doesn't support some of
>> the events?
> 
> Good question. I don't really have a great answer right now. Mostly it
> sort of depends on what events we end up. If it's something critical
> rdmavt will have to check during the registration and fail the call.
> This is not yet implemented in the two patch sets posted.
I'm not sure that's needed. If the driver doesn't satisfy what rvt
expects from it they can break in many ways. I'm not sure this specific
instance requires a check during registration. But it really depends on
the specific drivers and the specific events we're going to have.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dennis Dalessandro Dec. 10, 2015, 3:22 p.m. UTC | #6 
On Thu, Dec 10, 2015 at 05:12:52PM +0200, Haggai Eran wrote:
>On 10/12/2015 15:25, Dennis Dalessandro wrote:
>> On Thu, Dec 10, 2015 at 02:26:11PM +0200, Haggai Eran wrote:
>>> On 07/12/2015 22:43, Dennis Dalessandro wrote:
>>>> +    /*
>>>> +     * Drivers will need to support a number of notifications to rvt in
>>>> +     * accordance with certain events. This structure should contain
>>>> a mask
>>>> +     * of the supported events. Such events that the rvt may need to
>>>> know
>>>> +     * about include:
>>>> +     * port errors
>>>> +     * port active
>>>> +     * lid change
>>>> +     * sm change
>>>> +     * client reregister
>>>> +     * pkey change
>>> Where are the event values defined?
>> 
>> They aren't really yet. A lot of the comments like this were put in and
>> made available on GitHub for an early look into the design. This will
>> all get cleaned up as the code continues to come together.
>Okay, Great. Since the events above are already defined as
>ib_event/ib_event_type maybe you can reuse them somehow.

Yeah, I don't think there is a need to redefine this.

>>>>> +     *
>>>> +     * There may also be other events that the rvt layers needs to know
>>>> +     * about this is not an exhaustive list. Some events though rvt
>>>> does not
>>>> +     * need to rely on the driver for such as completion queue error.
>>>> +     */
>>>> +     int rvt_signal_supported;
>>>
>>> What will rvt do if it learns that the device doesn't support some of
>>> the events?
>> 
>> Good question. I don't really have a great answer right now. Mostly it
>> sort of depends on what events we end up. If it's something critical
>> rdmavt will have to check during the registration and fail the call.
>> This is not yet implemented in the two patch sets posted.
>I'm not sure that's needed. If the driver doesn't satisfy what rvt
>expects from it they can break in many ways. I'm not sure this specific
>instance requires a check during registration. But it really depends on
>the specific drivers and the specific events we're going to have.

Basically if the driver is just not going to function because it doesn't 
provide something, that's probably not such a big deal. If it can lead to a 
kernel panic then we probably want to validate. In the end, yes, it just all 
depends on what we end up with.

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/sw/rdmavt/pd.c b/drivers/infiniband/sw/rdmavt/pd.c
index 2f7a97f..1522880 100644
--- a/drivers/infiniband/sw/rdmavt/pd.c
+++ b/drivers/infiniband/sw/rdmavt/pd.c
@@ -72,7 +72,7 @@  struct ib_pd *rvt_alloc_pd(struct ib_device *ibdev,
 	 */
 
 	spin_lock(&dev->n_pds_lock);
-	if (dev->n_pds_allocated == dev->dparms.max_pds) {
+	if (dev->n_pds_allocated == dev->dparms.props.max_pd) {
 		spin_unlock(&dev->n_pds_lock);
 		kfree(pd);
 		ret = ERR_PTR(-ENOMEM);
diff --git a/include/rdma/rdma_vt.h b/include/rdma/rdma_vt.h
index b4ddcd3..89b8cd9 100644
--- a/include/rdma/rdma_vt.h
+++ b/include/rdma/rdma_vt.h
@@ -62,7 +62,45 @@ 
  * Things that are driver specific, module parameters in hfi1 and qib
  */
 struct rvt_driver_params {
-	int max_pds;
+	/*
+	 * driver required fields:
+	 *	node_guid
+	 *	phys_port_cnt
+	 *	dma_device
+	 *	owner
+	 * driver optional fields (rvt will provide generic value if blank):
+	 *	name
+	 *	node_desc
+	 * rvt fields, driver value ignored:
+	 *	uverbs_abi_ver
+	 *	node_type
+	 *	num_comp_vectors
+	 *	uverbs_cmd_mask
+	 */
+	struct ib_device_attr props;
+
+	/*
+	 * Drivers will need to support a number of notifications to rvt in
+	 * accordance with certain events. This structure should contain a mask
+	 * of the supported events. Such events that the rvt may need to know
+	 * about include:
+	 * port errors
+	 * port active
+	 * lid change
+	 * sm change
+	 * client reregister
+	 * pkey change
+	 *
+	 * There may also be other events that the rvt layers needs to know
+	 * about this is not an exhaustive list. Some events though rvt does not
+	 * need to rely on the driver for such as completion queue error.
+	 */
+	 int rvt_signal_supported;
+
+	/*
+	 * Anything driver specific that is not covered by props
+	 * For instance special module parameters. Goes here.
+	 */
 };
 
 /* Protection domain */
@@ -72,10 +110,28 @@  struct rvt_pd {
 };
 
 struct rvt_dev_info {
+	/*
+	 * Prior to calling for registration the driver will be responsible for
+	 * allocating space for this structure. The driver will also need to
+	 * allocate space for any private device or per port data structures.
+	 * Alternatively rvt could do this allocation and the registration API
+	 * would then change to accept an "extra" piece to allocate.
+	 *
+	 * The driver will also be
+	 * responsible for filling in certain members of dparms.props
+	 */
+
 	struct ib_device ibdev;
 
-	/* Driver specific */
+	/* Driver specific properties */
 	struct rvt_driver_params dparms;
+
+	/*
+	 * The work to create port files in /sys/class Infiniband is different
+	 * depending on the driver. This should not be extracted away and
+	 * instead drivers are responsible for setting the correct callback for
+	 * this.
+	 */
 	int (*port_callback)(struct ib_device *, u8, struct kobject *);
 
 	/* Internal use */