KVM will need information on the special handle mask used to indicate
emulated devices. In order to obtain this, a new type of list pci call
must be made to gather the information. Extend clp_list_pci_req to
also fetch the model-dependent-data field that holds this mask.
Signed-off-by: Matthew Rosato <[email protected]>
---
arch/s390/include/asm/pci.h | 1 +
arch/s390/include/asm/pci_clp.h | 2 +-
arch/s390/pci/pci_clp.c | 28 +++++++++++++++++++++++++---
3 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 00a2c24d6d2b..f3cd2da8128c 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
int clp_get_state(u32 fid, enum zpci_state *state);
int clp_refresh_fh(u32 fid, u32 *fh);
+int zpci_get_mdd(u32 *mdd);
/* UID */
void update_uid_checking(bool new);
diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
index 124fadfb74b9..d6bc324763f3 100644
--- a/arch/s390/include/asm/pci_clp.h
+++ b/arch/s390/include/asm/pci_clp.h
@@ -76,7 +76,7 @@ struct clp_req_list_pci {
struct clp_rsp_list_pci {
struct clp_rsp_hdr hdr;
u64 resume_token;
- u32 reserved2;
+ u32 mdd;
u16 max_fn;
u8 : 7;
u8 uid_checking : 1;
diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index bc7446566cbc..308ffb93413f 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
}
static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
- u64 *resume_token, int *nentries)
+ u64 *resume_token, int *nentries, u32 *mdd)
{
int rc;
@@ -354,6 +354,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
*nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
rrb->response.entry_size;
*resume_token = rrb->response.resume_token;
+ if (mdd)
+ *mdd = rrb->response.mdd;
return rc;
}
@@ -365,7 +367,7 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
int nentries, i, rc;
do {
- rc = clp_list_pci_req(rrb, &resume_token, &nentries);
+ rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
if (rc)
return rc;
for (i = 0; i < nentries; i++)
@@ -383,7 +385,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
int nentries, i, rc;
do {
- rc = clp_list_pci_req(rrb, &resume_token, &nentries);
+ rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
if (rc)
return rc;
fh_list = rrb->response.fh_list;
@@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
return rc;
}
+int zpci_get_mdd(u32 *mdd)
+{
+ struct clp_req_rsp_list_pci *rrb;
+ u64 resume_token = 0;
+ int nentries, rc;
+
+ if (!mdd)
+ return -EINVAL;
+
+ rrb = clp_alloc_block(GFP_KERNEL);
+ if (!rrb)
+ return -ENOMEM;
+
+ rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
+
+ clp_free_block(rrb);
+ return rc;
+}
+EXPORT_SYMBOL_GPL(zpci_get_mdd);
+
static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
{
unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);
--
2.27.0
On 1/14/22 21:31, Matthew Rosato wrote:
> KVM will need information on the special handle mask used to indicate
> emulated devices. In order to obtain this, a new type of list pci call
> must be made to gather the information. Extend clp_list_pci_req to
> also fetch the model-dependent-data field that holds this mask.
>
> Signed-off-by: Matthew Rosato <[email protected]>
> ---
> arch/s390/include/asm/pci.h | 1 +
> arch/s390/include/asm/pci_clp.h | 2 +-
> arch/s390/pci/pci_clp.c | 28 +++++++++++++++++++++++++---
> 3 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 00a2c24d6d2b..f3cd2da8128c 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
> int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
> int clp_get_state(u32 fid, enum zpci_state *state);
> int clp_refresh_fh(u32 fid, u32 *fh);
> +int zpci_get_mdd(u32 *mdd);
>
> /* UID */
> void update_uid_checking(bool new);
> diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
> index 124fadfb74b9..d6bc324763f3 100644
> --- a/arch/s390/include/asm/pci_clp.h
> +++ b/arch/s390/include/asm/pci_clp.h
> @@ -76,7 +76,7 @@ struct clp_req_list_pci {
> struct clp_rsp_list_pci {
> struct clp_rsp_hdr hdr;
> u64 resume_token;
> - u32 reserved2;
> + u32 mdd;
> u16 max_fn;
> u8 : 7;
> u8 uid_checking : 1;
> diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
> index bc7446566cbc..308ffb93413f 100644
> --- a/arch/s390/pci/pci_clp.c
> +++ b/arch/s390/pci/pci_clp.c
> @@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
> }
>
> static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
> - u64 *resume_token, int *nentries)
> + u64 *resume_token, int *nentries, u32 *mdd)
> {
> int rc;
>
> @@ -354,6 +354,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
> *nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
> rrb->response.entry_size;
> *resume_token = rrb->response.resume_token;
> + if (mdd)
> + *mdd = rrb->response.mdd;
>
> return rc;
> }
> @@ -365,7 +367,7 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
> int nentries, i, rc;
>
> do {
> - rc = clp_list_pci_req(rrb, &resume_token, &nentries);
> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
> if (rc)
> return rc;
> for (i = 0; i < nentries; i++)
> @@ -383,7 +385,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
> int nentries, i, rc;
>
> do {
> - rc = clp_list_pci_req(rrb, &resume_token, &nentries);
> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
> if (rc)
> return rc;
> fh_list = rrb->response.fh_list;
> @@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
> return rc;
> }
>
> +int zpci_get_mdd(u32 *mdd)
> +{
> + struct clp_req_rsp_list_pci *rrb;
> + u64 resume_token = 0;
> + int nentries, rc;
> +
> + if (!mdd)
> + return -EINVAL;
I think this tests is not useful.
The caller must take care not to call with a NULL pointer,
what the only caller today make sure.
> +
> + rrb = clp_alloc_block(GFP_KERNEL);
> + if (!rrb)
> + return -ENOMEM;
> +
> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
> +
> + clp_free_block(rrb);
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(zpci_get_mdd);
> +
> static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
> {
> unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);
>
--
Pierre Morel
IBM Lab Boeblingen
On Tue, 18 Jan 2022 11:36:06 +0100
Pierre Morel <[email protected]> wrote:
> On 1/14/22 21:31, Matthew Rosato wrote:
> > KVM will need information on the special handle mask used to indicate
> > emulated devices. In order to obtain this, a new type of list pci call
> > must be made to gather the information. Extend clp_list_pci_req to
> > also fetch the model-dependent-data field that holds this mask.
> >
> > Signed-off-by: Matthew Rosato <[email protected]>
> > ---
> > arch/s390/include/asm/pci.h | 1 +
> > arch/s390/include/asm/pci_clp.h | 2 +-
> > arch/s390/pci/pci_clp.c | 28 +++++++++++++++++++++++++---
> > 3 files changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > index 00a2c24d6d2b..f3cd2da8128c 100644
> > --- a/arch/s390/include/asm/pci.h
> > +++ b/arch/s390/include/asm/pci.h
> > @@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
> > int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
> > int clp_get_state(u32 fid, enum zpci_state *state);
> > int clp_refresh_fh(u32 fid, u32 *fh);
> > +int zpci_get_mdd(u32 *mdd);
> >
> > /* UID */
> > void update_uid_checking(bool new);
> > diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
> > index 124fadfb74b9..d6bc324763f3 100644
> > --- a/arch/s390/include/asm/pci_clp.h
> > +++ b/arch/s390/include/asm/pci_clp.h
> > @@ -76,7 +76,7 @@ struct clp_req_list_pci {
> > struct clp_rsp_list_pci {
> > struct clp_rsp_hdr hdr;
> > u64 resume_token;
> > - u32 reserved2;
> > + u32 mdd;
> > u16 max_fn;
> > u8 : 7;
> > u8 uid_checking : 1;
> > diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
> > index bc7446566cbc..308ffb93413f 100644
> > --- a/arch/s390/pci/pci_clp.c
> > +++ b/arch/s390/pci/pci_clp.c
> > @@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
> > }
> >
> > static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
> > - u64 *resume_token, int *nentries)
> > + u64 *resume_token, int *nentries, u32 *mdd)
> > {
> > int rc;
> >
> > @@ -354,6 +354,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
> > *nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
> > rrb->response.entry_size;
> > *resume_token = rrb->response.resume_token;
> > + if (mdd)
> > + *mdd = rrb->response.mdd;
> >
> > return rc;
> > }
> > @@ -365,7 +367,7 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
> > int nentries, i, rc;
> >
> > do {
> > - rc = clp_list_pci_req(rrb, &resume_token, &nentries);
> > + rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
> > if (rc)
> > return rc;
> > for (i = 0; i < nentries; i++)
> > @@ -383,7 +385,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
> > int nentries, i, rc;
> >
> > do {
> > - rc = clp_list_pci_req(rrb, &resume_token, &nentries);
> > + rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
> > if (rc)
> > return rc;
> > fh_list = rrb->response.fh_list;
> > @@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
> > return rc;
> > }
> >
> > +int zpci_get_mdd(u32 *mdd)
> > +{
> > + struct clp_req_rsp_list_pci *rrb;
> > + u64 resume_token = 0;
> > + int nentries, rc;
> > +
> > + if (!mdd)
> > + return -EINVAL;
>
> I think this tests is not useful.
> The caller must take care not to call with a NULL pointer,
> what the only caller today make sure.
what if the caller does it anyway?
I think the test is useful. if passing NULL is a bug, then maybe
consider using BUG_ON, or WARN_ONCE
>
>
> > +
> > + rrb = clp_alloc_block(GFP_KERNEL);
> > + if (!rrb)
> > + return -ENOMEM;
> > +
> > + rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
> > +
> > + clp_free_block(rrb);
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(zpci_get_mdd);
> > +
> > static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
> > {
> > unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);
> >
>
On Fri, 2022-01-14 at 15:31 -0500, Matthew Rosato wrote:
> KVM will need information on the special handle mask used to indicate
> emulated devices. In order to obtain this, a new type of list pci call
> must be made to gather the information. Extend clp_list_pci_req to
> also fetch the model-dependent-data field that holds this mask.
>
> Signed-off-by: Matthew Rosato <[email protected]>
> ---
> arch/s390/include/asm/pci.h | 1 +
> arch/s390/include/asm/pci_clp.h | 2 +-
> arch/s390/pci/pci_clp.c | 28 +++++++++++++++++++++++++---
> 3 files changed, 27 insertions(+), 4 deletions(-)
>
---8<---
>
> +int zpci_get_mdd(u32 *mdd)
> +{
> + struct clp_req_rsp_list_pci *rrb;
> + u64 resume_token = 0;
> + int nentries, rc;
> +
> + if (!mdd)
> + return -EINVAL;
> +
> + rrb = clp_alloc_block(GFP_KERNEL);
> + if (!rrb)
> + return -ENOMEM;
> +
> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
> +
> + clp_free_block(rrb);
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(zpci_get_mdd);
> +
> static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
> {
> unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);
Looks good.
Reviewed-by: Niklas Schnelle <[email protected]>
On 1/26/22 11:13, Claudio Imbrenda wrote:
> On Tue, 18 Jan 2022 11:36:06 +0100
> Pierre Morel <[email protected]> wrote:
>
>> On 1/14/22 21:31, Matthew Rosato wrote:
>>> KVM will need information on the special handle mask used to indicate
>>> emulated devices. In order to obtain this, a new type of list pci call
>>> must be made to gather the information. Extend clp_list_pci_req to
>>> also fetch the model-dependent-data field that holds this mask.
>>>
>>> Signed-off-by: Matthew Rosato <[email protected]>
>>> ---
>>> arch/s390/include/asm/pci.h | 1 +
>>> arch/s390/include/asm/pci_clp.h | 2 +-
>>> arch/s390/pci/pci_clp.c | 28 +++++++++++++++++++++++++---
>>> 3 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>>> index 00a2c24d6d2b..f3cd2da8128c 100644
>>> --- a/arch/s390/include/asm/pci.h
>>> +++ b/arch/s390/include/asm/pci.h
>>> @@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
>>> int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
>>> int clp_get_state(u32 fid, enum zpci_state *state);
>>> int clp_refresh_fh(u32 fid, u32 *fh);
>>> +int zpci_get_mdd(u32 *mdd);
>>>
>>> /* UID */
>>> void update_uid_checking(bool new);
>>> diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
>>> index 124fadfb74b9..d6bc324763f3 100644
>>> --- a/arch/s390/include/asm/pci_clp.h
>>> +++ b/arch/s390/include/asm/pci_clp.h
>>> @@ -76,7 +76,7 @@ struct clp_req_list_pci {
>>> struct clp_rsp_list_pci {
>>> struct clp_rsp_hdr hdr;
>>> u64 resume_token;
>>> - u32 reserved2;
>>> + u32 mdd;
>>> u16 max_fn;
>>> u8 : 7;
>>> u8 uid_checking : 1;
>>> diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
>>> index bc7446566cbc..308ffb93413f 100644
>>> --- a/arch/s390/pci/pci_clp.c
>>> +++ b/arch/s390/pci/pci_clp.c
>>> @@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
>>> }
>>>
>>> static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
>>> - u64 *resume_token, int *nentries)
>>> + u64 *resume_token, int *nentries, u32 *mdd)
>>> {
>>> int rc;
>>>
>>> @@ -354,6 +354,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
>>> *nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
>>> rrb->response.entry_size;
>>> *resume_token = rrb->response.resume_token;
>>> + if (mdd)
>>> + *mdd = rrb->response.mdd;
>>>
>>> return rc;
>>> }
>>> @@ -365,7 +367,7 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
>>> int nentries, i, rc;
>>>
>>> do {
>>> - rc = clp_list_pci_req(rrb, &resume_token, &nentries);
>>> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>>> if (rc)
>>> return rc;
>>> for (i = 0; i < nentries; i++)
>>> @@ -383,7 +385,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
>>> int nentries, i, rc;
>>>
>>> do {
>>> - rc = clp_list_pci_req(rrb, &resume_token, &nentries);
>>> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>>> if (rc)
>>> return rc;
>>> fh_list = rrb->response.fh_list;
>>> @@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
>>> return rc;
>>> }
>>>
>>> +int zpci_get_mdd(u32 *mdd)
>>> +{
>>> + struct clp_req_rsp_list_pci *rrb;
>>> + u64 resume_token = 0;
>>> + int nentries, rc;
>>> +
>>> + if (!mdd)
>>> + return -EINVAL;
>>
>> I think this tests is not useful.
>> The caller must take care not to call with a NULL pointer,
>> what the only caller today make sure.
>
> what if the caller does it anyway?
>
> I think the test is useful. if passing NULL is a bug, then maybe
> consider using BUG_ON, or WARN_ONCE
I think generally the caller is responsible for the test.
In our case for example the caller can use directly the address
of a u32 allocated on the stack or globaly and he knows if a test is
useful or not.
Of course we can systematically check in every kernel function all
pointer parameters against NULL.
But this is not userland, not even a inter-architecture core function
and we can expect the kernel programmer to programm correctly.
For our special case zpci_get_mdd() nor clp_list_pci_req() do access
*mdd if mdd is NULL so that giving a NULL mdd pointer will not trigger a
fault.
Also, the function is named zpci_get_mdd(u32 *mdd) if the caller do not
give a pointer to mdd which would be quite stupid to call a function
zpci_get_mdd() in this circumstance he will just no get mdd, no side effect.
So the only purpose having this test here is to say the caller that he
forgot to check his mdd allocation.
My opinion is that he should have check.
>
>>
>>
>>> +
>>> + rrb = clp_alloc_block(GFP_KERNEL);
>>> + if (!rrb)
>>> + return -ENOMEM;
>>> +
>>> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
>>> +
>>> + clp_free_block(rrb);
>>> + return rc;
>>> +}
>>> +EXPORT_SYMBOL_GPL(zpci_get_mdd);
>>> +
>>> static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
>>> {
>>> unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);
>>>
>>
>
--
Pierre Morel
IBM Lab Boeblingen
On 1/27/22 8:41 AM, Pierre Morel wrote:
>
>
> On 1/26/22 11:13, Claudio Imbrenda wrote:
>> On Tue, 18 Jan 2022 11:36:06 +0100
>> Pierre Morel <[email protected]> wrote:
>>
>>> On 1/14/22 21:31, Matthew Rosato wrote:
>>>> KVM will need information on the special handle mask used to indicate
>>>> emulated devices. In order to obtain this, a new type of list pci call
>>>> must be made to gather the information. Extend clp_list_pci_req to
>>>> also fetch the model-dependent-data field that holds this mask.
>>>>
>>>> Signed-off-by: Matthew Rosato <[email protected]>
>>>> ---
>>>> arch/s390/include/asm/pci.h | 1 +
>>>> arch/s390/include/asm/pci_clp.h | 2 +-
>>>> arch/s390/pci/pci_clp.c | 28 +++++++++++++++++++++++++---
>>>> 3 files changed, 27 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>>>> index 00a2c24d6d2b..f3cd2da8128c 100644
>>>> --- a/arch/s390/include/asm/pci.h
>>>> +++ b/arch/s390/include/asm/pci.h
>>>> @@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32
>>>> *fh, u8 nr_dma_as);
>>>> int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
>>>> int clp_get_state(u32 fid, enum zpci_state *state);
>>>> int clp_refresh_fh(u32 fid, u32 *fh);
>>>> +int zpci_get_mdd(u32 *mdd);
>>>> /* UID */
>>>> void update_uid_checking(bool new);
>>>> diff --git a/arch/s390/include/asm/pci_clp.h
>>>> b/arch/s390/include/asm/pci_clp.h
>>>> index 124fadfb74b9..d6bc324763f3 100644
>>>> --- a/arch/s390/include/asm/pci_clp.h
>>>> +++ b/arch/s390/include/asm/pci_clp.h
>>>> @@ -76,7 +76,7 @@ struct clp_req_list_pci {
>>>> struct clp_rsp_list_pci {
>>>> struct clp_rsp_hdr hdr;
>>>> u64 resume_token;
>>>> - u32 reserved2;
>>>> + u32 mdd;
>>>> u16 max_fn;
>>>> u8 : 7;
>>>> u8 uid_checking : 1;
>>>> diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
>>>> index bc7446566cbc..308ffb93413f 100644
>>>> --- a/arch/s390/pci/pci_clp.c
>>>> +++ b/arch/s390/pci/pci_clp.c
>>>> @@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
>>>> }
>>>> static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
>>>> - u64 *resume_token, int *nentries)
>>>> + u64 *resume_token, int *nentries, u32 *mdd)
>>>> {
>>>> int rc;
>>>> @@ -354,6 +354,8 @@ static int clp_list_pci_req(struct
>>>> clp_req_rsp_list_pci *rrb,
>>>> *nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
>>>> rrb->response.entry_size;
>>>> *resume_token = rrb->response.resume_token;
>>>> + if (mdd)
>>>> + *mdd = rrb->response.mdd;
>>>> return rc;
>>>> }
>>>> @@ -365,7 +367,7 @@ static int clp_list_pci(struct
>>>> clp_req_rsp_list_pci *rrb, void *data,
>>>> int nentries, i, rc;
>>>> do {
>>>> - rc = clp_list_pci_req(rrb, &resume_token, &nentries);
>>>> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>>>> if (rc)
>>>> return rc;
>>>> for (i = 0; i < nentries; i++)
>>>> @@ -383,7 +385,7 @@ static int clp_find_pci(struct
>>>> clp_req_rsp_list_pci *rrb, u32 fid,
>>>> int nentries, i, rc;
>>>> do {
>>>> - rc = clp_list_pci_req(rrb, &resume_token, &nentries);
>>>> + rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>>>> if (rc)
>>>> return rc;
>>>> fh_list = rrb->response.fh_list;
>>>> @@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
>>>> return rc;
>>>> }
>>>> +int zpci_get_mdd(u32 *mdd)
>>>> +{
>>>> + struct clp_req_rsp_list_pci *rrb;
>>>> + u64 resume_token = 0;
>>>> + int nentries, rc;
>>>> +
>>>> + if (!mdd)
>>>> + return -EINVAL;
>>>
>>> I think this tests is not useful.
>>> The caller must take care not to call with a NULL pointer,
>>> what the only caller today make sure.
>>
>> what if the caller does it anyway?
>>
>> I think the test is useful. if passing NULL is a bug, then maybe
>> consider using BUG_ON, or WARN_ONCE
>
> I think generally the caller is responsible for the test.
> In our case for example the caller can use directly the address
> of a u32 allocated on the stack or globaly and he knows if a test is
> useful or not.
>
> Of course we can systematically check in every kernel function all
> pointer parameters against NULL.
> But this is not userland, not even a inter-architecture core function
> and we can expect the kernel programmer to programm correctly.
I appreciate your optimism :)
>
> For our special case zpci_get_mdd() nor clp_list_pci_req() do access
> *mdd if mdd is NULL so that giving a NULL mdd pointer will not trigger a
> fault.
> Also, the function is named zpci_get_mdd(u32 *mdd) if the caller do not
> give a pointer to mdd which would be quite stupid to call a function
> zpci_get_mdd() in this circumstance he will just no get mdd, no side
> effect.
> So the only purpose having this test here is to say the caller that he
> forgot to check his mdd allocation.
> My opinion is that he should have check.
Based on the thread of conversation, I'm going to assume you meant 'My
opinion that he should -not- have the check here'
So, I'm generally a fan of being a bit defensive in paths that are not
performance-intensive and would therefore typically tend to agree with
Claudio. But in this particular case, I'm willing to just drop this
check and move on, primarily because 1) we only have a single caller
already checking this case and 2) I don't really anticipate any
additional callers later.
@Niklas do you care / is it OK to keep your R-b if I remove this
if(!mdd) check?